Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/17/2015 03:59 PM, Andrew Cooper wrote:
> On 15/09/15 10:19, Razvan Cojocaru wrote:
>> Previously, if vm_event emulation support was enabled, then REP
>> optimizations were disabled when emulating REP-compatible
>> instructions. This patch allows fine-tuning of this behaviour by
>> providing a dedicated libxc helper function.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> This disables all rep optimisations by default, so on its own is
> inappropriate.

REP optimizations are enabled by default. Emulate_each_rep is initially
set to 0, when struct hvm_domain is being initialized, which means that
REP optimizations are enabled. I've tested this and it does work, am I
missing something?

> I am also not sure that an individual domctl subop is appropriate.  Its
> purpose is to undo a performance hit caused by introspection, so should
> live as an introspection subop IMO.

Do you mean xc_monitor_emulate_each_rep() instead of
xc_domain_emulate_each_rep()?

I've placed this in its own domctl subop because it's not introspection
(or vm_event) specific. The change in
xen/arch/x86/hvm/emulate.c enables / disables REP emulation
optimizations regardless of whether there's a vm_event client or not. I
thought this might come handy for somebody else too.

>> ---
>>  tools/libxc/include/xenctrl.h    |   11 +++++++++++
>>  tools/libxc/xc_domain.c          |   18 ++++++++++++++++++
>>  xen/arch/x86/hvm/emulate.c       |    2 +-
>>  xen/common/domctl.c              |    5 +++++
>>  xen/include/asm-x86/hvm/domain.h |    1 +
>>  xen/include/public/domctl.h      |    8 ++++++++
>>  6 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 3482544..4ece851 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -643,6 +643,17 @@ int xc_domain_node_getaffinity(xc_interface *xch,
>>                                 xc_nodemap_t nodemap);
>>  
>>  /**
>> + * This function enables / disables emulation for each REP for a
>> + * REP-compatible instruction.
>> + *
>> + * @parm xch a handle to an open hypervisor interface.
>> + * @parm domid the domain id one wants to get the node affinity of.
>> + * @parm enable if 0 optimize when possible, else emulate each REP.
>> + * @return 0 on success, -1 on failure.
>> + */
>> +int xc_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable);
>> +
>> +/**
>>   * This function specifies the CPU affinity for a vcpu.
>>   *
>>   * There are two kinds of affinity. Soft affinity is on what CPUs a vcpu
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index e7278dd..19b2e46 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -2555,6 +2555,24 @@ int xc_domain_soft_reset(xc_interface *xch,
>>      domctl.domain = (domid_t)domid;
>>      return do_domctl(xch, &domctl);
>>  }
>> +
>> +int xc_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable)
>> +{
>> +    int ret = -1;
>> +    DECLARE_DOMCTL;
>> +
>> +    domctl.cmd = XEN_DOMCTL_emulate_each_rep;
>> +    domctl.domain = (domid_t)domid;
>> +    domctl.u.emulate_each_rep.op = enable;
>> +
>> +    ret = do_domctl(xch, &domctl);
>> +
>> +    if ( ret == -ESRCH )
>> +        errno = ENOENT;
> 
> Why do you override ESRCH?  ESRCH is the expected error for a missing
> domain.

I shouldn't, really. This is a copy/paste issue - I've copied and
changed a similar function in Xen 4.4, and that code just got carried
over to this patch. I'll remove that check.


Thanks,
Razvan