Re: [PATCH v5 05/15] x86/altp2m: basic data structures and support routines.

["Sahita, Ravi" <ravi.sahita@intel.com>]

>From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
>Sent: Tuesday, July 14, 2015 8:57 AM
>
>On 07/14/2015 01:14 AM, Ed White wrote:
>> Add the basic data structures needed to support alternate p2m's and
>> the functions to initialise them and tear them down.
>>
>> Although Intel hardware can handle 512 EPTP's per hardware thread
>> concurrently, only 10 per domain are supported in this patch for
>> performance reasons.
>>
>> The iterator in hap_enable() does need to handle 512, so that is now
>> uint16_t.
>>
>> This change also splits the p2m lock into one lock type for altp2m's
>> and another type for all other p2m's. The purpose of this is to place
>> the altp2m list lock between the types, so the list lock can be
>> acquired whilst holding the host p2m lock.
>>
>> Signed-off-by: Ed White <edmund.h.white@intel.com>
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>With the number of major changes you made here, you definitely should
>have dropped this reviewed-by.
>
>> ---
>>  xen/arch/x86/hvm/Makefile        |   1 +
>>  xen/arch/x86/hvm/altp2m.c        |  77 +++++++++++++++++++++++++++++
>>  xen/arch/x86/hvm/hvm.c           |  21 ++++++++
>>  xen/arch/x86/mm/hap/hap.c        |  38 ++++++++++++++-
>>  xen/arch/x86/mm/mm-locks.h       |  46 +++++++++++++++++-
>>  xen/arch/x86/mm/p2m.c            | 102
>+++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-x86/domain.h     |  10 ++++
>>  xen/include/asm-x86/hvm/altp2m.h |  38 +++++++++++++++
>>  xen/include/asm-x86/hvm/hvm.h    |  14 ++++++
>>  xen/include/asm-x86/hvm/vcpu.h   |   9 ++++
>>  xen/include/asm-x86/p2m.h        |  32 +++++++++++-
>>  11 files changed, 384 insertions(+), 4 deletions(-)  create mode
>> 100644 xen/arch/x86/hvm/altp2m.c  create mode 100644
>> xen/include/asm-x86/hvm/altp2m.h
>>
>> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
>> index 69af47f..eb1a37b 100644
>> --- a/xen/arch/x86/hvm/Makefile
>> +++ b/xen/arch/x86/hvm/Makefile
>> @@ -1,6 +1,7 @@
>>  subdir-y += svm
>>  subdir-y += vmx
>>
>> +obj-y += altp2m.o
>>  obj-y += asid.o
>>  obj-y += emulate.o
>>  obj-y += event.o
>> diff --git a/xen/arch/x86/hvm/altp2m.c b/xen/arch/x86/hvm/altp2m.c new
>> file mode 100644 index 0000000..a10f347
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/altp2m.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#include <asm/hvm/support.h>
>> +#include <asm/hvm/hvm.h>
>> +#include <asm/p2m.h>
>> +#include <asm/hvm/altp2m.h>
>> +
>> +void
>> +altp2m_vcpu_reset(struct vcpu *v)
>
>OK, so it looks like at the end of this patch series:
>* altp2m_vcpu_reset() isn't called outside this file
>* altp2m_vcpu_initialise() is only called from hvm.c when the guest enables
>the altp2m functionality
>* altp2m_vcpu_destroy() is called when the guest disables altp2m
>funcitonality, or when the vcpu is destroyed
>
>Looking at the "vcpu_destroy" case, it's hard to tell exactly how much on that
>path is actually useful; but it looks like the only thing that's critical is decreasing
>the active_vcpu count of the p2m that's being used.
>
>Also, it looks like these functions don't do anything specifically with the HVM
>side of things.
>
>So on the whole, it seems like these would better go along with the other
>altp2m functions inside p2m.c.
>
>Thoughts?

George, apologies on this one - I completely missed this email from you -

We could move these functions into p2m.c, except destroy, the VMCS updates are critical.
We will try to get this into what will be our final rev (at least for 4.6 candidate). 
Again sorry for the snafu on this email.

Ravi

>
> -George