Re: [Xen-devel] [PATCH V5 07/12] xen: Introduce monitor_op domctl

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

On Tue, Feb 17, 2015 at 7:20 PM, Tamas K Lengyel
<tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 13.02.15 at 17:33, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -411,7 +411,8 @@ static int hvmemul_virtual_to_linear(
>>>       * being triggered for repeated writes to a whole page.
>>>       */
>>>      *reps = min_t(unsigned long, *reps,
>>> -                  
>>> unlikely(current->domain->arch.hvm_domain.introspection_enabled)
>>> +                  unlikely(current->domain->arch
>>> +                            .monitor_options.mov_to_msr.extended_capture)
>>>                             ? 1 : 4096);
>>
>> This makes no sense (especially not to a reader in a year or two):
>> There's no connection between mov-to-msr and the repeat count
>> capping done here. Please wrap this in a suitably named is_...() or
>> has_...() or introspection_enabled() helper, with a comment at its
>> definition site making the connection explicit.
>
> It took me a while to understand what "introspection_enabled" actually
> represents and all it really does at the moment is enabling the
> interception of an extended set of MSRs. If something, that is a bad
> variable name. Since in this series "introspection_enabled" is
> removed, here I just updated the variable to the one that holds the
> same information. I don't actually know what the code here does as I
> didn't touch it. If this indeed has no connection to mov-to-msr, it
> should have its own option field with its own name actually describing
> what it does. Maybe Razvan has some more information on what is going
> on here and if another variable needs to be introduced that was just
> latched onto "introspection_enabled".

So I looked into this a bit more and this is being used when a
mem_event response to a mem_access event has the emulation flag set.
So this is an extra option that was latched onto
introspection_enabled, thus we will need an extra field to determine
if this particular feature is enabled or not.

Now I understand why Razvan was wondering about "umbrella" options
going forward. IMHO this highlights the problem with umbrella options
- it becomes really hard to understand what the option actually does.
Especially without proper documentation.

Tamas

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