From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v5 05/15] x86/altp2m: basic data structures
 and support routines.
Date: Tue, 14 Jul 2015 15:58:52 +0100
Message-ID: <55A53FCC0200007800090D41@mail.emea.novell.com>
References: <1436832903-12639-1-git-send-email-edmund.h.white@intel.com>
	<1436832903-12639-6-git-send-email-edmund.h.white@intel.com>
	<55A527140200007800090B74@mail.emea.novell.com>
	<55A52089.1050007@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <55A52089.1050007@eu.citrix.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: George Dunlap <george.dunlap@eu.citrix.com>, Ed White <edmund.h.white@intel.com>
Cc: Tim Deegan <tim@xen.org>, Ravi Sahita <ravi.sahita@intel.com>, Wei Liu <wei.liu2@citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org, tlengyel@novetta.com, Daniel De Graaf <dgdegra@tycho.nsa.gov>
List-Id: xen-devel@lists.xenproject.org

>>> On 14.07.15 at 16:45, <george.dunlap@eu.citrix.com> wrote:
> On 07/14/2015 02:13 PM, Jan Beulich wrote:
>>>>> On 14.07.15 at 02:14, <edmund.h.white@intel.com> wrote:
>>> +void
>>> +altp2m_vcpu_initialise(struct vcpu *v)
>>> +{
>>> +    if ( v != current )
>>> +        vcpu_pause(v);
>>> +
>>> +    altp2m_vcpu_reset(v);
>>> +    vcpu_altp2m(v).p2midx = 0;
>>> +    atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
>>> +
>>> +    altp2m_vcpu_update_eptp(v);
>>> +
>>> +    if ( v != current )
>>> +        vcpu_unpause(v);
>>> +}
>>> +
>>> +void
>>> +altp2m_vcpu_destroy(struct vcpu *v)
>>> +{
>>> +    struct p2m_domain *p2m;
>>> +
>>> +    if ( v != current )
>>> +        vcpu_pause(v);
>>> +
>>> +    if ( (p2m = p2m_get_altp2m(v)) )
>>> +        atomic_dec(&p2m->active_vcpus);
>>> +
>>> +    altp2m_vcpu_reset(v);
>>> +
>>> +    altp2m_vcpu_update_eptp(v);
>>> +    altp2m_vcpu_update_vmfunc_ve(v);
>>> +
>>> +    if ( v != current )
>>> +        vcpu_unpause(v);
>>> +}
>> 
>> There not being any caller of altp2m_vcpu_initialise() I can't judge
>> about its pausing requirements, but for the destroy case I can't
>> see what the pausing is good for. Considering its sole user it's also
>> not really clear why the two update operations need to be done
>> while destroying.
> 
> So looking at this after all the patches have been applied, it looks
> like initialise() and destroy() are called from the
> altp2m_set_domain_state(), which the guest uses to enable or disable the
> altp2m functionality.  In that case, it seems like pausing is probably
> appropriate.

Right. Albeit I'd then question whether it wouldn't better be the caller
to do the pausing (via domain_pause_except_self()), considering that
the functions are being called in a fir_each_vcpu() loop.

Jan