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

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

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