Re: [Xen-devel] [RFC PATCH V3 10/12] xen: Introduce monitor_op domctl

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 01/30/2015 12:38 PM, Tamas K Lengyel wrote:
> On Fri, Jan 30, 2015 at 8:58 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 01/29/2015 11:46 PM, Tamas K Lengyel wrote:
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index c7a0bde..3b58700 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -1445,15 +1445,6 @@ void p2m_vm_event_emulate_check(struct vcpu *v, 
>>> const vm_event_response_t *rsp)
>>>      }
>>>  }
>>>
>>> -void p2m_setup_introspection(struct domain *d)
>>> -{
>>> -    if ( hvm_funcs.enable_msr_exit_interception )
>>> -    {
>>> -        d->arch.hvm_domain.introspection_enabled = 1;
>>> -        hvm_funcs.enable_msr_exit_interception(d);
>>> -    }
>>> -}
>>> -
>>>  bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>                              struct npfec npfec,
>>>                              vm_event_request_t **req_ptr)
>>
>> I see that introspection_enabled is no longer assigned here ...
> 
> Introspection_enabled is getting deprecated in this patch and is moved
> into the monitor_op domctl.

Moved, right, but where?

This patch removes the d->arch.hvm_domain.introspection_enabled = 1
assignment from here but AFAICT it doesn't move it anwyhere else. It
just removes it, so introspection_enabled is always 0.

I couldn't find any place in this series where introspection_enabled =
1. Could you please point it out to me if I've missed it by accident?

>>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>>> index 0db899e..0b30750 100644
>>> --- a/xen/common/vm_event.c
>>> +++ b/xen/common/vm_event.c
>>> @@ -617,16 +617,10 @@ int vm_event_domctl(struct domain *d, 
>>> xen_domctl_vm_event_op_t *vec,
>>>          switch( vec->op )
>>>          {
>>>          case XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE:
>>> -        case XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE_INTROSPECTION:
>>>          {
>>>              rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
>>>                                      HVM_PARAM_MONITOR_RING_PFN,
>>>                                      mem_access_notification);
>>> -
>>> -            if ( vec->op == 
>>> XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE_INTROSPECTION
>>> -                 && !rc )
>>> -                p2m_setup_introspection(d);
>>> -
>>>          }
>>>          break;
>>>
>>> @@ -635,7 +629,6 @@ int vm_event_domctl(struct domain *d, 
>>> xen_domctl_vm_event_op_t *vec,
>>>              if ( ved->ring_page )
>>>              {
>>>                  rc = vm_event_disable(d, ved);
>>> -                d->arch.hvm_domain.introspection_enabled = 0;
>>>              }
>>>          }
>>>          break;
>>
>> ... nor here. Patch 6/12 checks it but doesn't set it. Patch 5/12 sets
>> it to 0 (which could account for the removal of the assignment in
>> vm_event.c) but never to 1. A few important things depend on it being
>> enabled: it becomes impossible to disable interception for a select set
>> of MSRs, optimization for RET instructions emulation is disabled, and
>> othere places in p2m.c makes use of the flag as well.
>>
>> Is there some place in the code, untouched by this series, where
>> introspection_enabled is being set to 1?
> 
> It is moved into the monitor_op domctl when mov_to_msr trapping is
> enabled. The reason of having introspection_enabled AFAIU was to
> reenable trapping MSR's that were disabled shortly after boot. Thus,
> an option field is present in the monitor_op when enabling mov_to_msr
> trapping: extended_capture. Let me know if this still achieves the
> same effect as before!

No, there are several places where introspection_enabled is needed, as
I've said before. Not just the MSRs.

One important place is in hvmemul_virtual_to_linear(), in
xen/arch/x86/hvm/emulate.c, where we disable optimizations for REP
instructions (in today's version of mainline Xen, at line 413).

There are also places in patches yet to be published I've worked on
where we gate things on introspection_enabled being != 0, so please
don't remove it or have it misbehave.

I'll look into the extended_capture option in case it will allow future
removal of the MSR special case for introspection, but a flag like that
is necessary and can't simply be deprecated and removed.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel