Re: [PATCH 04/12] Nested Virtualization: core

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

On Monday 27 December 2010 08:54:16 Dong, Eddie wrote:
> Dong, Eddie wrote:
> > # HG changeset patch
> > # User cegger
> > # Date 1292839432 -3600
> > Nested Virtualization core implementation
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> >
> > diff -r e43ab6fb0ee2 -r a9465de5a794 xen/arch/x86/hvm/Makefile
> > --- a/xen/arch/x86/hvm/Makefile
> > +++ b/xen/arch/x86/hvm/Makefile
> > @@ -10,6 +10,7 @@ obj-y += intercept.o
> >  obj-y += io.o
> >  obj-y += irq.o
> >  obj-y += mtrr.o
> > +obj-y += nestedhvm.o
> >  obj-y += pmtimer.o
> >  obj-y += quirks.o
> >  obj-y += rtc.o
> > diff -r e43ab6fb0ee2 -r a9465de5a794 xen/arch/x86/hvm/nestedhvm.c
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/nestedhvm.c
> > @@ -0,0 +1,198 @@
> > +/*
> > + * Nested HVM
> > + * Copyright (c) 2010, Advanced Micro Devices, Inc.
> > + * Author: Christoph Egger <Christoph.Egger@amd.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it + * under the terms and conditions of the GNU General
> > Public License, + * version 2, as published by the Free Software
> > Foundation. + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > General Public License for + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > along with + * this program; if not, write to the Free Software
> > Foundation, Inc., 59 Temple + * Place - Suite 330, Boston, MA
> > 02111-1307 USA. + */
> > +
> > +#include <asm/msr.h>
> > +#include <asm/hvm/support.h>	/* for HVM_DELIVER_NO_ERROR_CODE */
> > +#include <asm/hvm/hvm.h>
> > +#include <asm/hvm/nestedhvm.h>
> > +#include <asm/event.h>  /* for local_event_delivery_(en|dis)able */
> > +#include <asm/paging.h> /* for paging_mode_hap() */
> > +
> > +
> > +/* Nested HVM on/off per domain */
> > +bool_t
> > +nestedhvm_enabled(struct domain *d)
> > +{
> > +    bool_t enabled;
> > +
> > +    enabled = !!(d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM]);
> > +    /* sanity check */
> > +    BUG_ON(enabled && !is_hvm_domain(d));
> > +
> > +    if (!is_hvm_domain(d))
> > +        return 0;
> > +
> > +    return enabled;
> > +}
> > +
> > +/* Nested VCPU */
> > +bool_t
> > +nestedhvm_vcpu_in_guestmode(struct vcpu *v)
> > +{
> > +    return vcpu_nestedhvm(v).nv_guestmode;
> > +}
> > +
> > +void
> > +nestedhvm_vcpu_reset(struct vcpu *v)
> > +{
> > +    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
> > +
> > +    if (nv->nv_vmcx)
> > +        hvm_unmap_guest_frame(nv->nv_vmcx);
> > +    nv->nv_vmcx = NULL;
> > +    nv->nv_vmcxaddr = VMCX_EADDR;
> > +    nv->nv_flushp2m = 0;
> > +    nv->nv_p2m = NULL;
> > +
> > +    nhvm_vcpu_reset(v);
> > +
> > +    /* vcpu is in host mode */
> > +    nestedhvm_vcpu_exit_guestmode(v);
> > +}
> > +
> > +int
> > +nestedhvm_vcpu_initialise(struct vcpu *v)
> > +{
> > +    int rc;
> > +    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
> > +
> > +    if (!nestedhvm_enabled(v->domain))
> > +        return 0;
> > +
> > +    memset(nv, 0x0, sizeof(struct nestedvcpu));
> > +
> > +    /* initialise hostsave, for example */
> > +    rc = nhvm_vcpu_initialise(v);
> > +    if (rc) {
> > +        nhvm_vcpu_destroy(v);
> > +        return rc;
> > +    }
> > +
> > +    nestedhvm_vcpu_reset(v);
> > +    return 0;
> > +}
> > +
> > +int
> > +nestedhvm_vcpu_destroy(struct vcpu *v)
> > +{
> > +    if (!nestedhvm_enabled(v->domain))
> > +        return 0;
> > +
> > +    return nhvm_vcpu_destroy(v);
> > +}
> > +
> > +void
> > +nestedhvm_vcpu_enter_guestmode(struct vcpu *v)
> > +{
> > +    vcpu_nestedhvm(v).nv_guestmode = 1;
> > +}
> > +
> > +void
> > +nestedhvm_vcpu_exit_guestmode(struct vcpu *v)
> > +{
> > +    vcpu_nestedhvm(v).nv_guestmode = 0;
> > +}
> > +
> > +/* Common shadow IO Permission bitmap */
> > +
> > +struct shadow_iomap {
> > +    /* same format and size as hvm_io_bitmap */
> > +    unsigned long iomap[3*PAGE_SIZE/BYTES_PER_LONG];
> > +    int refcnt;
> > +};
> > +
> > +/* There four global patterns of io bitmap each guest can
> > + * choose one of them depending on interception of io port 0x80
> > and/or + * 0xED (shown in table below). Each shadow iomap pattern is
> > + * implemented as a singleton to minimize memory consumption while
> > + * providing a provider/consumer interface to the users.
> > + * The users are in SVM/VMX specific code.
> > + *
> > + * bitmap        port 0x80  port 0xed
> > + * hvm_io_bitmap cleared    cleared
> > + * iomap[0]      cleared    set
> > + * iomap[1]      set        cleared
> > + * iomap[2]      set        set
> > + */
> > +static struct shadow_iomap *nhvm_io_bitmap[3];
> > +
> > +unsigned long *
> > +nestedhvm_vcpu_iomap_get(bool_t port_80, bool_t port_ed)
> > +{
> > +    int i;
> > +    extern int hvm_port80_allowed;
> > +
> > +    if (!hvm_port80_allowed)
> > +        port_80 = 1;
> > +
> > +    if (port_80 == 0) {
> > +        if (port_ed == 0)
> > +            return hvm_io_bitmap;
> > +        i = 0;
> > +    } else {
> > +        if (port_ed == 0)
> > +            i = 1;
> > +        else
> > +            i = 2;
> > +    }
> > +
> > +    if (nhvm_io_bitmap[i] == NULL) {
> > +        nhvm_io_bitmap[i] =
> > +            _xmalloc(sizeof(struct shadow_iomap), PAGE_SIZE);
> > +        nhvm_io_bitmap[i]->refcnt = 0;
> > +        /* set all bits */
> > +        memset(nhvm_io_bitmap[i]->iomap, ~0,
> > sizeof(nhvm_io_bitmap[i]->iomap)); +        switch (i) {
> > +        case 0:
> > +            __clear_bit(0x80, nhvm_io_bitmap[i]->iomap);
> > +            break;
> > +        case 1:
> > +            __clear_bit(0xed, nhvm_io_bitmap[i]->iomap);
> > +            break;
> > +        case 2:
> > +            break;
> > +        }
> > +    }
> > +
>
> This is overcomplicated. Static table should serve this much simple and
> efficient.

The logic to select the right static table will be still needed. I am not sure
if removing the _xmalloc() call simplifies this part a lot.

I appreciate opinions from other people on this.

Christoph



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