Re: [PATCH 13/13] Nested Virtualization: hap-on-hap

[Christoph Egger <Christoph.Egger@amd.com>]

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