Re: [Xen-devel] [PATCH v7 05/15] x86/altp2m: basic data structures and support routines.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.15 at 01:01, <edmund.h.white@xxxxxxxxx> wrote:
> @@ -6569,6 +6571,25 @@ void hvm_toggle_singlestep(struct vcpu *v)
>      v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
>  }
>  
> +void altp2m_vcpu_update_p2m(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_update_p2m )
> +        hvm_funcs.altp2m_vcpu_update_p2m(v);
> +}
> +
> +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
> +        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
> +}
> +
> +bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
> +        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
> +    return 0;
> +}

These are _still_ not inline functions (in hvm.h), and 00/15 also
doesn't mention that this was intentionally left out.

> @@ -498,6 +498,28 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> +    if ( hvm_altp2m_supported() )
> +    {
> +        /* Init alternate p2m data */
> +        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +        {
> +            rv = -ENOMEM;
> +            goto out;
> +        }
> +
> +        for ( i = 0; i < MAX_EPTP; i++ )
> +            d->arch.altp2m_eptp[i] = INVALID_MFN;

And there is _still_ EPT-specific code left in a generic file.

> @@ -183,6 +184,43 @@ static void p2m_teardown_nestedp2m(struct domain *d)
>      }
>  }
>  
> +static void p2m_teardown_altp2m(struct domain *d)
> +{
> +    unsigned int i;
> +    struct p2m_domain *p2m;
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( !d->arch.altp2m_p2m[i] )
> +            continue;
> +        p2m = d->arch.altp2m_p2m[i];
> +        p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;

If you already think it's necessary to latch the array entry into a
local variable, why don't you zap the array entry _before_ freeing
the structure?

> @@ -1940,6 +1988,59 @@ int unmap_mmio_regions(struct domain *d,
>      return err;
>  }
>  
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> +{
> +    struct p2m_domain *p2m;
> +    struct ept_data *ept;
> +    unsigned int i;
> +
> +    altp2m_list_lock(d);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +        ept = &p2m->ept;
> +
> +        if ( eptp == ept_get_eptp(ept) )
> +            goto out;
> +    }
> +
> +    i = INVALID_ALTP2M;
> +
> +out:

Just to repeat - labels should be indented by at least one space.

And you already know what the comment is regarding this being
EPT-specific code (and here one can't even debate whether it's
just unfortunate naming, since ept_get_eptp() is _very_ EPT-
specific, and that macro - if headers were properly structured -
shouldn't even be visible here).

> --- /dev/null
> +++ b/xen/include/asm-x86/altp2m.h
> @@ -0,0 +1,38 @@
> +/*
> + * Alternate p2m HVM
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * 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.
> + */
> +
> +#ifndef _ALTP2M_H
> +#define _ALTP2M_H

I'm sure I said so before - this is not a specific enough guard symbol.
It should have at least an _X86 prefix.

> +#include <xen/types.h>
> +#include <xen/sched.h>         /* for struct vcpu, struct domain */
> +#include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */

I don't see the type mentioned in the comment used anywhere
below - is this a stray include?

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -233,6 +233,10 @@ struct paging_vcpu {
>  typedef xen_domctl_cpuid_t cpuid_input_t;
>  
>  #define MAX_NESTEDP2M 10
> +
> +#define MAX_ALTP2M      ((uint16_t)10)
> +#define INVALID_ALTP2M  ((uint16_t)~0)

Didn't you claim to have removed all stray casts?

Considering how late we are with this, this patch can have my ack
only provided you promise to address _all_ of the issues above in
follow-up patches.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel