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 13/13] Nested Virtualization: hap-on-hap

To: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
From: Christoph Egger <Christoph.Egger@xxxxxxx>
Date: Thu, 2 Dec 2010 18:49:16 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 02 Dec 2010 09:52:04 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20101116165825.GG25462@xxxxxxxxxxxxxxxxxxxxxxx>
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: <201011121945.57564.Christoph.Egger@xxxxxxx> <20101116165825.GG25462@xxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.9.10
On Tuesday 16 November 2010 17:58:25 Tim Deegan wrote:
> Hi,
>
> This patch doesn't address most of my concerns with the last version.
>
> My comments about callers of gva_to_gfn still stand -- basically,
> gva_to_gfn should return an N1 GFN, and most callers (including hvm_copy)
> should not need to know about N2 GFNs or multiple p2ms.

Done.

>
> Also, you're still copying large parts of the existing HAP walker
> (e.g. the next_level function).  Can you avoid the duplication by
> using a write_p2m_entry() callback, since that seems to be the part
> that's different?

Yes, I did it with a callback.

>
> Skipping a flush when p2m->cr3 == 0 is not safe.  I suggested using -1
> as the magic value last time.

Done.

>
> My comments on why p2m_flush_locked() isn't enough to reclaim an in-use
> p2m for recycling still stand.

Can you point me to the mail in the ML archive you refer to, please?

>
> I have one more comment on the new code:
>
> At 18:45 +0000 on 12 Nov (1289587557), Christoph Egger wrote:
> >  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_valid(omfn) ) {
>
> I think you need to do this flush even if !mfn_valid(omfn) - for example
> if you're removing a mapping of an MMIO area.

Fixed.

>
> > +            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_valid(nmfn)
> > +                && !(l1e_get_flags(new) & _PAGE_PRESENT) )
>
> I don't understand this test at all - you need to flush if you're
> removing an old present entry regardless of what replaces it. The only
> case where you can skip the flush is if old == new.

Fixed.

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



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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