From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH RFC 0/7] xen: Clean-up of mem_event subsystem Date: Mon, 17 Nov 2014 18:43:40 +0200 Message-ID: <546A25BC.3020000@bitdefender.com> References: <1415806309-5206-1-git-send-email-tamas.lengyel@zentific.com> <54638141.3010500@citrix.com> <546A32720200007800048873@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <546A32720200007800048873@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper , xen-devel@lists.xen.org, Tamas K Lengyel Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, eddie.dong@intel.com, ian.jackson@eu.citrix.com, andres@lagarcavilla.org, jun.nakajima@intel.com, rshriram@cs.ubc.ca, dgdegra@tycho.nsa.gov, yanghy@cn.fujitsu.com List-Id: xen-devel@lists.xenproject.org On 11/17/2014 06:37 PM, Jan Beulich wrote: >>>> On 12.11.14 at 16:48, wrote: >> On 12/11/14 15:31, Tamas K Lengyel wrote: >>> xen/include/public/domctl.h | 44 +-- >>> xen/include/public/hvm/params.h | 2 +- >>> xen/include/public/mem_event.h | 134 ------- >>> xen/include/public/memory.h | 6 +- >>> xen/include/public/vm_event.h | 179 +++++++++ >> >> While in principle I think this series is a very good thing, there is a >> problem with editing the pubic header files. >> >> The contents of mem_event.h is not currently hidden behind #ifdef >> __XEN_TOOLS__ >> >> As a result, it is strictly speaking part of the VM-visible public >> API/ABI and not permitted to change in a backwards incompatible manor. >> >> Having said that, it is currently only usable by privileged domains, so >> there is an argument to be made for declaring that it should have been >> hidden behind __XEN_TOOLS__ in the first place, making it permittable to >> change. > > I'm not sure I agree - the meaning of "tools" here would seem quite a > bit different than e.g. in domctl.h. Looking at patch 1, I can't see how > an old consumer (remember that for many of these we have at best > fake consumers in the tree) would deal with the now differently > arranged data. I don't see any versioning of the interface, and hence > I can't see how tools would know which of the formats to expect. In the initial patch I've sent Tamas I had arranged things as follows, (so that the layout would stay compatible) but I think we ended up discussing it and deciding it would look cleaner to just re-do the whole thing: +struct mem_event_ept_data { + uint64_t gfn; + uint64_t offset; + uint64_t gla; /* if gla_valid */ +}; + +struct mem_event_cr_data { + uint64_t new_value; + uint64_t _pad; + uint64_t old_value; +}; + +struct mem_event_int3_data { + uint64_t gfn; + uint64_t _pad; + uint64_t eip; +}; + +struct mem_event_singlestep_data { + uint64_t gfn; + uint64_t _pad; + uint64_t eip; +}; + +struct mem_event_msr_data { + uint64_t msr; + uint64_t old_value; + uint64_t new_value; +}; + typedef struct mem_event_st { uint32_t flags; uint32_t vcpu_id; - uint64_t gfn; - uint64_t offset; - uint64_t gla; /* if gla_valid */ + union { + struct mem_event_ept_data ept_event; + struct mem_event_cr_data cr_event; + struct mem_event_int3_data int3_event; + struct mem_event_singlestep_data singlestep_event; + struct mem_event_msr_data msr_event; + }; uint32_t p2mt; Would something like this be more along the right lines? Thanks, Razvan