WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 12/12] Nested Virtualization: hap-on-hap

To: Christoph Egger <Christoph.Egger@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH 12/12] Nested Virtualization: hap-on-hap
From: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Date: Tue, 22 Mar 2011 14:59:37 +0000
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 22 Mar 2011 08:01:11 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <201103091531.04219.Christoph.Egger@xxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <201103091531.04219.Christoph.Egger@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.20 (2009-06-14)
Hi, 

Looks like you've sorted out shooting down old users of a p2m table. 
hap_write_p2m_entry still isn't right, though:

> @@ -834,38 +864,81 @@ static void
>  hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p,
>                      mfn_t table_mfn, l1_pgentry_t new, unsigned int level)
>  {
> -    uint32_t old_flags;
> +    struct domain *d = v->domain;
> +    uint32_t old_flags = l1e_get_flags(*p);

You have moved this read outside the hap_lock.  Please put it back. 

> +    p2m_type_t op2mt = p2m_flags_to_type(old_flags);
>  
> -    hap_lock(v->domain);
> +    /* We know always use the host p2m here, regardless if the vcpu
> +     * is in host or guest mode. The vcpu can be in guest mode by
> +     * a hypercall which passes a domain and chooses mostly the first
> +     * vcpu.
> +     * XXX This is the reason why this function can not be used re-used
> +     * for updating the nestedp2m. Otherwise, hypercalls would randomly
> +     * operate on host p2m and nested p2m.
> +     */
> +    if ( nestedhvm_enabled(d)
> +      && p2m_is_valid(op2mt) )
> +    {
> +        if ( l1e_get_intpte(new) != l1e_get_intpte(*p) ) {
> +            p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new));
>  
> -    old_flags = l1e_get_flags(*p);
> +            /* Skip flush on vram tracking or XP mode in Win7 hang
> +             * very early in the virtual BIOS (long before the bootloader
> +             * runs), otherwise. VRAM tracking happens so often that
> +             * flushing and fixing the nestedp2m doesn't let XP mode
> +             * proceed to boot.
> +             */
> +            if ( !((op2mt == p2m_ram_rw && np2mt == p2m_ram_logdirty)
> +                || (op2mt == p2m_ram_logdirty && np2mt == p2m_ram_rw)) )

That's not safe.  If the MFN has changed, you _have_ to flush, even if
you're replacing a logdirty entry with a r/w one.   And if you're
replacing a r/w entry with a logdirty one, you _have_ to flush or
logdirty doesn't work correctly.  If that case is too slow then you
should batch the flushes somehow, not just skip them.

Cheers,

Tim.

> +            {
> +                /* This GFN -> MFN is going to get removed. */
> +                /* XXX There is a more efficient way to do that
> +                 * but it works for now.
> +                 * Note, p2m_flush_nestedp2m calls hap_lock() internally.
> +                 */
> +                p2m_flush_nestedp2m(d);
> +            }
> +        }
> +    }
> +
> +    hap_lock(d);
> +
>      safe_write_pte(p, new);
>      if ( (old_flags & _PAGE_PRESENT)
>           && (level == 1 || (level == 2 && (old_flags & _PAGE_PSE))) )
> -             flush_tlb_mask(&v->domain->domain_dirty_cpumask);
> +             flush_tlb_mask(&d->domain_dirty_cpumask);
>  
>  #if CONFIG_PAGING_LEVELS == 3
>      /* install P2M in monitor table for PAE Xen */
>      if ( level == 3 )
>          /* We have written to the p2m l3: need to sync the per-vcpu
>           * copies of it in the monitor tables */
> -        p2m_install_entry_in_monitors(v->domain, (l3_pgentry_t *)p);
> +        p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
>  #endif
>  
> -    hap_unlock(v->domain);
> +    hap_unlock(d);
>  }

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>