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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 01/30/2015 01:15 PM, Tamas K Lengyel wrote:
> On Fri, Jan 30, 2015 at 12:07 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 01/30/2015 12:38 PM, Tamas K Lengyel wrote:
>>> On Fri, Jan 30, 2015 at 8:58 AM, Razvan Cojocaru
>>> <rcojocaru@bitdefender.com> 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.

I see, but is it conceivable that some vm_event consumer does want to
enable arch.monitor_options.mov_to_msr.extended_capture but not be
interested in doing full-blown introspection (for example, is fine with
having the REP optimization enabled)?

What you're proposing here (as far as introspection_enabled is
concerned) is, if I understand correctly, basically renaming
introspection_enabled to mov_to_msr.extended_capture. I can see how that
would seem to simplify some things, but it might not look very intuitive
to future developers, and it will definitely complicate other things
down the road.

An example is havig the guest trigger a vm_event, requested by the
privileged userspace application. In our case, it was VMCALL in the
original series, and the patch has been eventually dropped from the
conversation - to be reworked at a later time. But we do need it, and
our patch now does its thing gated on introspection_enabled. It's hard
to connect that logic to mov_to_msr.extended_capture.

I guess we could keep the flag and move it to arch.monitor_options if
you prefer? And setting it could cause mov_to_msr.extended_capture and
assorted flags to be set also (some sort of umbrella setting)?


Thanks,
Razvan