From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 1/3] [RFC] x86/hvm: Permit
 HVM_PARAM_IDENT_PT to be set more than once
Date: Fri, 8 May 2015 14:48:45 +0100
Message-ID: <554CBEBD.3050902@citrix.com>
References: <1431089675-31163-1-git-send-email-andrew.cooper3@citrix.com>
	<1431089675-31163-2-git-send-email-andrew.cooper3@citrix.com>
	<554CDA6802000078000784D8@mail.emea.novell.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: <554CDA6802000078000784D8@mail.emea.novell.com>
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: Jan Beulich <JBeulich@suse.com>
Cc: Yang Hongyang <yanghy@cn.fujitsu.com>, Keir Fraser <keir@xen.org>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 08/05/15 14:46, Jan Beulich wrote:
>>>> On 08.05.15 at 14:54, <andrew.cooper3@citrix.com> wrote:
>> There is no conceptual problem with setting this parameter more than once.
>> Checkpointed migration streams will typically set it once per checkpoint to
>> the same value.
>>
>> The parameter is only actually needed on early-generation VT-x which lacked
>> the unrestricted guest capability, although it could plausibly be used on
>> newer VT-x with unusual execution control settings.  Short circuit the
>> expensive operations on non VT-x hardware.
>>
>> The parameter itself must always be latched to avoid issues if the VM
>> eventually migrates to a host which needs to use the pagetable.
> Reads all plausible, except that I think the way you carry out this
> last aspect needs adjustment:
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5730,12 +5730,13 @@ static int hvmop_set_param(
>>              rc = -EINVAL;
>>          break;
>>      case HVM_PARAM_IDENT_PT:
>> -        rc = -EINVAL;
>> -        if ( d->arch.hvm_domain.params[a.index] != 0 )
>> -            break;
>> -
>>          rc = 0;
>> -        if ( !paging_mode_hap(d) )
>> +        d->arch.hvm_domain.params[a.index] = a.value;
>> +        /*
>> +         * Only actually required for VT-x lacking unrestricted_guest
>> +         * capabilities.  Short circuit the pause if possible.
>> +         */
>> +        if ( !paging_mode_hap(d) || !cpu_has_vmx )
>>              break;
> You should latch the new value inside this if()'s body and ...
>
>> @@ -5749,7 +5750,6 @@ static int hvmop_set_param(
>>  
>>          rc = 0;
>>          domain_pause(d);
>> -        d->arch.hvm_domain.params[a.index] = a.value;
>>          for_each_vcpu ( d, v )
>>              paging_update_cr3(v);
>>          domain_unpause(d);
> ... leave this one alone so the use site in VMX code won't see the
> new value prematurely.

Good point - I will fix this up.

~Andrew