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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 17.02.15 at 19:20, <tamas.lengyel@zentific.com> wrote:
> On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 13.02.15 at 17:33, <tamas.lengyel@zentific.com> wrote:
>>>              rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
>>> -                                    HVM_PARAM_MONITOR_RING_PFN,
>>> -                                    mem_access_notification);
>>> -
>>> -            if ( vec->op == XEN_VM_EVENT_MONITOR_ENABLE_INTROSPECTION
>>> -                 && !rc )
>>> -                p2m_setup_introspection(d);
>>> -
>>> -        }
>>> -        break;
>>> +                                 HVM_PARAM_MONITOR_RING_PFN,
>>> +                                 mem_access_notification);
>>
>> I don't see what changes for these two lines. If it's indentation, it
>> should be done right when the code gets added.
> 
> Indentation can't be fixed in the code addition as it breaks git -M.
> It reverts to the old format where it just removes the whole file and
> adds the new one. I think its a waste to add a whole new separate
> patch just to fix indentations so I just fix it here.

Considering that indentation is broken already prior to your
series, this is perhaps acceptable. But at least if indentation
was correct before the rename, it should be afterwards. You'd
have to use of git's -B option to control the resulting diff.

>>> +#include <public/domctl.h>
>>> +
>>> +static inline
>>> +int monitor_domctl(struct xen_domctl_monitor_op *op, struct domain *d)
>>
>> The includes above are insufficient for the types used, or you should
>> forward declare _both_ structs and not have any includes.
> 
> Just including sched.h additionally should be enough IMHO.

Resulting in a huge pile of further dependencies. Our goal really
should be to get the dependencies down, not up - improving build
time. Hence forward declarations are very likely the better choice
here.

>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -241,6 +241,24 @@ struct time_scale {
>>>      u32 mul_frac;
>>>  };
>>>
>>> +/************************************************/
>>> +/*            monitor event options             */
>>> +/************************************************/
>>> +struct mov_to_cr {
>>> +    uint8_t enabled;
>>> +    uint8_t sync;
>>> +    uint8_t onchangeonly;
>>> +};
>>> +
>>> +struct mov_to_msr {
>>> +    uint8_t enabled;
>>> +    uint8_t extended_capture;
>>> +};
>>> +
>>> +struct debug_event {
>>> +    uint8_t enabled;
>>> +};
>>
>> These are all internal structures - is there anything wrong with using
>> bitfields here?
> 
> The use if bitfields is not good performance-wise AFAIK. Would there
> be any benefit that would offset that?

As Andrew already said - total structure size. Also I'm pretty
convinced "or $<val>, <mem>" as well as "and $~<val>,<mem>"
aren't much worse than "mov $<val>,<mem>", and the code
writing these fields shouldn't be performance critical. And
"test $<val>,<mem>" and "cmp $<val>,<mem>" (as well as
their split up alternatives, should the compiler elect to do so)
ought to be equal performance wise.

Jan