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

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 13:26 -0800 on 09 Jan (1420806395), Ed White wrote:
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -22,4 +22,5 @@ obj-y += vlapic.o
>  obj-y += vmsi.o
>  obj-y += vpic.o
>  obj-y += vpt.o
> -obj-y += vpmu.o
> \ No newline at end of file
> +obj-y += vpmu.o
> +obj-y += altp2mhvm.o

This list is in alphabetical order; please add this at the top. :)

> +    /* Init alternate p2m data */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    {
> +        rv = -ENOMEM;
> +        goto out;
> +    }
> +    for (i = 0; i < 512; i++)
> +        d->arch.altp2m_eptp[i] = ~0ul;

This 512 is architectural, I guess?  It should have a named constant.

> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -209,6 +209,10 @@ declare_mm_lock(nestedp2m)
>  #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
>  #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
>  
> +declare_mm_lock(altp2m)
> +#define altp2m_lock(d)   mm_lock(altp2m, &(d)->arch.altp2m_lock)
> +#define altp2m_unlock(d) mm_unlock(&(d)->arch.altp2m_lock)

This needs a nice big block comment describing what it protects, like
the other locks in this file.  (Urgh, I see that the nested-p2m lock
has come unmoored from its comment; I'll fix that now).

> +struct altp2mvcpu {
> +    uint16_t    p2midx ;        /* alternate p2m index */
> +    uint64_t    veinfo;         /* #VE information page guest pfn */

This is a gfn, I think?  I think I'd prefer 'unsigned long veinfo_gfn'
for clarity.

Cheers,

Tim.

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