[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Fri, Jan 30, 2015 at 12:07 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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.

Actually it should have been removed altogether from the HVM domain
definition as the same information should be stored in arch_domain's
monitor_options struct.

>
> 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.

Ack, the plan was actually to replace all references to
arch.hvm_domain.introspection_enabled with
arch.monitor_options.mov_to_msr.extended_capture. I see I forgot to
actually fully follow through that plan but that's the intention at
least. So the functionality would remain, it would just be worked into
the coherent settings field with every other type of events.

>
>
> Thanks,
> Razvan

Thanks,
Tamas

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.