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

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

Hi, 

We still haven't resolved the issue of safely recycling a p2m table that
is in use on another CPU.  I'll quote my last email in its entirety
here:

| At 16:27 +0000 on 20 Dec (1292862443), Christoph Egger wrote:
| > > > An other vcpu is in VMRUN emulation after a nestedp2m is assigned.
| > > >    It will VMEXIT with a nested page fault.
| > >
| > > Why?
| > 
| > Because the p2m is empty. The MMU can not do a page table walk.
| > 
| > > > An other vcpu already running l2 guest.
| > > >    It will VMEXIT with a nested page fault immediately.
| > >
| > > Hmm.  It will exit for the TLB shootdown IPI, but I think you need
| to
| > > clear vcpu_nestedhvm(v).nh_p2m on the other vcpu to make sure it
| doesn't
| > > re-enter with the p2m you've just recycled.
| > 
| > The p2m is empty so I don't see a problem when it gets recycled.
| 
| It's only empty very briefly.  You've assigned it to a vcpu which is
| about to take a nested fault and fill it with entries, right?
| 
| What happens if the other vcpu is handling an SMI or executing a tight
| loop of register arithmetic for a few thousand cycles?  What stops it
| seeing the new contents of the p2m?

One other issue I pointed out last time is still there in this patch:

> @@ -835,38 +865,76 @@ 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)
>  {
> +    struct domain *d = v->domain;
>      uint32_t old_flags;
>  
> -    hap_lock(v->domain);
> +    old_flags = l1e_get_flags(*p);
>  
> -    old_flags = l1e_get_flags(*p);
> +    /* 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) ) {
> +        mfn_t omfn = _mfn(l1e_get_pfn(*p));
> +        p2m_type_t op2mt = p2m_flags_to_type(old_flags);
> +
> +        if ( p2m_is_valid(op2mt) ) {
> +            mfn_t nmfn = _mfn(l1e_get_pfn(new));
> +            p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new));
> +
> +            if ( p2m_is_valid(np2mt) && (mfn_x(omfn) != mfn_x(nmfn)) ) {

Checking that the mfns are the same isn't quite enough, as the
permissions might have changed (e.g. log-dirty mode removing write
access).  You need to test for (new == *p) to be sure that you don't
need to flush.

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


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