From mboxrd@z Thu Jan  1 00:00:00 1970
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH V2 1/2] xen,
 libxc: Fine grained control of REP emulation optimizations
Date: Tue, 22 Sep 2015 18:28:28 +0300
Message-ID: <5601739C.6010304@bitdefender.com>
References: <1442842294-6123-1-git-send-email-rcojocaru@bitdefender.com>
	<1442842294-6123-2-git-send-email-rcojocaru@bitdefender.com>
	<56018D1302000078000A4822@prv-mh.provo.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: <56018D1302000078000A4822@prv-mh.provo.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: tamas@tklengyel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@eu.citrix.co, stefano.stabellini@citrix.com, keir@xen.org
List-Id: xen-devel@lists.xenproject.org

On 09/22/2015 06:17 PM, Jan Beulich wrote:
>>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
>>       * being triggered for repeated writes to a whole page.
>>       */
>>      *reps = min_t(unsigned long, *reps,
>> -                  unlikely(current->domain->arch.mem_access_emulate_enabled)
>> +                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
>> +                           current->domain->arch.mem_access_emulate_each_rep)
>>                             ? 1 : 4096);
> 
> unlikely() should not wrap compound conditions, or else its effect of
> eliminating mis-predicted branches from the fast path won't be
> achieved. In the case here I wonder though whether you couldn't
> simply test only ->arch.mem_access_emulate_each_rep.

I'll unfold the unlikely().

Testing only ->arch.mem_access_emulate_each_rep is what I had done in
the original patch, however on Andrew Cooper's suggestion I've now gated
this on ->domain->arch.mem_access_emulate_enabled as well.

Otherwise, somebody might set mem_access_emulate_each_rep via its
xc_monitor_*() call, but then after calling xc_monitor_disable() it
would still be in effect, even if the guest is no longer being monitored.

If this is not a problem, I'm happy to check just
->arch.mem_access_emulate_each_rep.

>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -79,6 +79,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>>          return 0;
>>      }
>>  
>> +    if ( mop->op == XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP )
>> +    {
>> +        d->arch.mem_access_emulate_each_rep = !!mop->event;
>> +        return 0;
>> +    }
> 
> Considering that there's another "if(mop->op == ...)" right above
> this, these two together should become another switch().

Understood.


Thanks,
Razvan