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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Jan 30, 2015 at 1:24 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 01/30/2015 01:45 PM, Tamas K Lengyel wrote:
>>>>
>>>> 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)?
>>
>> I'm not sure I follow what the difference here between
>> mov_to_msr.extended_capture and what you refer to as "full-blown
>> introspection". My understanding was that without gating some MSR's
>> via introspection_enabled, they are never trapped into the hypervisor
>> when they get written to, while other MSR do. Thus the two options:
>> mov_to_msr.enabled for basic trapping, and mov_to_msr.extended_capture
>> for gating the extended set of MSRs to be also trappable.
>
> Well, write interception for the MSRs we're interested in is enabled
> _initally_, but then vmx_disable_intercept_for_msr() gets called and
> we're left without these events, so yes, in a nutshell we do want to
> keep write events for more MSRs than the default allows around.
>
> The initial, very basic patch:
>
> http://lists.xenproject.org/archives/html/xen-devel/2014-07/msg00310.html
>
> The point was to keep receiving these events even after the HV decides
> the corresponding MSR writes have become unnecessarily expensive to
> intercept, but only if the vm_event consumer is interested in guest
> introspection.
>
> By "full-blown" introspection I mean that, in order to do its job
> properly, guest introspection needs a bunch of requirements to be met
> (not just the proper MSR write events to be sent), not least of which is
> to send an event per each write performed in a REP situation (not just
> an event per page touched) - which is why, gated on introspection being
> enabled, we disable the REP emulation optimization.
>
> It also needs a number of other things, which will surely become RFCs
> here at some point.
>
> Hence my previous comments: full-blown vm_event-based introspection is
> more than the MSR trick, so it might be slightly confusing to gate all
> of them on mov_to_msr.extended_capture.

OK, I see what you mean now.

>
>>> 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.
>>
>> No, not just simply renaming it. So far the options for the various VM
>> events were scattered all over the place, yours defined directly on
>> hvm_domain, others in hvm parameters. Now there is an assigned spot
>> for all current - and future - events to store their settings: in
>> arch_domain monitor_settings. This will work for PV domains as well,
>> while keeping it on the arch_domain layer will avoid defining options
>> on ARM that are architecture specific (mov_to_cr0/3/4 etc.).
>>
>> Furthermore, setting these options was also a mess which I try to
>> address in this patch: some were enabled via hvm_op hypercalls, yours
>> via domctl. Now everything is moved into one spot: monitor_op domctl.
>
> Yes, and that's great! Your efforts are appreciated, and I'm sorry to
> have inadvertently contributed to the mess.
>
> I was simply talking about the way introspection_enabled is being used:
> you were taking before about replacing all instances of
> "introspection_enabled" with "mov_to_msr.enabled" (which is functionally
> speaking, renaming), and my only points
> have been that:
>
> A) in this series introspection_enabled has neither been removed nor is
> it of any use, since it's always 0, and
>
> B) exclusive use of mov_to_msr.extended_capture where
> introspection_enabled has been used before might not logically cover all
> the cases needed for proper guest introspection (not sure that gating
> the code that disables the REP emulation optimization on
> mov_to_msr.extended_capture wouldn't look strange to someone looking at
> the code for the first time).
>
> I hope I've been able to explain myself better this time, perhaps my
> concerns are not shared by others, I'm certainly open to discussion.

It's a question whether we need an umbrella option for this, or is it
more appropriate to check for each required event being enabled
individually. At the moment I don't see the need for it but in the
future going forward I can see it being added if it makes the code
easier to read. It would however also add some extra complexity: what
happens if an event is disabled that is covered by the umbrella
option? Would we disable all other events under the umbrella, or just
the umbrella option itself? So yes, it's definitely a discussion we
should have, when the need arrives for it.

>
>>> 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)?
>>
>> Yes, that would be the logic going forward - all VM event related
>> options and settings should be stored in this structure. Also, I don't
>> see a problem with having an event or setting that enables multiple at
>> the same time.
>
> That's great, thanks! That would pretty much shut me up. :)
>
>
> Thanks,
> Razvan

Thanks,
Tamas

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