From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V5 07/12] xen: Introduce monitor_op domctl Date: Wed, 18 Feb 2015 09:26:00 +0000 Message-ID: <54E468B80200007800060F1B@mail.emea.novell.com> References: <1423845203-18941-1-git-send-email-tamas.lengyel@zentific.com> <1423845203-18941-8-git-send-email-tamas.lengyel@zentific.com> <54E357FE02000078000609D7@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Tim Deegan , Kevin Tian , "wei.liu2@citrix.com" , Ian Campbell , Steven Maresca , Stefano Stabellini , Eddie Dong , Ian Jackson , "xen-devel@lists.xen.org" , Andres Lagar-Cavilla , Jun Nakajima , "rshriram@cs.ubc.ca" , Keir Fraser , Daniel De Graaf , "yanghy@cn.fujitsu.com" List-Id: xen-devel@lists.xenproject.org >>> On 17.02.15 at 19:20, wrote: > On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich wrote: >>>>> On 13.02.15 at 17:33, 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 >>> + >>> +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 $, " as well as "and $~," aren't much worse than "mov $,", and the code writing these fields shouldn't be performance critical. And "test $," and "cmp $," (as well as their split up alternatives, should the compiler elect to do so) ought to be equal performance wise. Jan