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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.

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

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

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