[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH V2 1/8] xen/mem_event: Cleanup of mem_event structures
>>> On 18.01.15 at 16:17, <tamas.lengyel@xxxxxxxxxxxx> wrote: > --- a/xen/include/Makefile > +++ b/xen/include/Makefile > @@ -90,7 +90,7 @@ ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH)) > > all: headers.chk > > -headers.chk: $(filter-out public/arch-% public/%ctl.h public/xsm/% > public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) $(public-y)) Makefile > +headers.chk: $(filter-out public/arch-% public/%ctl.h public/mem_event.h > public/xsm/% public/%hvm/save.h, $(wildcard public/*.h public/*/*.h) > $(public-y)) Makefile I think you should finally break this already too long line. But of course first of all you'll want to explain why the addition is necessary/correct. The mere fact that this now becomes a tools-only interface isn't enough imo - some of the other headers excluded here would better undergo the checking too, just that first they would need cleaning up. > --- a/xen/include/public/mem_event.h > +++ b/xen/include/public/mem_event.h > @@ -27,9 +27,15 @@ > #ifndef _XEN_PUBLIC_MEM_EVENT_H > #define _XEN_PUBLIC_MEM_EVENT_H > > +#if !defined(__XEN__) && !defined(__XEN_TOOLS__) > +#error "vm event operations are intended for use only by Xen or node > control tools" > +#endif > + > #include "xen.h" > #include "io/ring.h" > > +#define MEM_EVENT_INTERFACE_VERSION 0x00000001 This doesn't appear to be used anywhere, and hence isn't useful to add here. > @@ -48,16 +54,27 @@ > */ > #define MEM_EVENT_FLAG_EMULATE_NOWRITE (1 << 6) > > -/* Reasons for the memory event request */ > -#define MEM_EVENT_REASON_UNKNOWN 0 /* typical reason */ > -#define MEM_EVENT_REASON_VIOLATION 1 /* access violation, GFN is > address */ > -#define MEM_EVENT_REASON_CR0 2 /* CR0 was hit: gfn is new CR0 > value, gla is previous */ > -#define MEM_EVENT_REASON_CR3 3 /* CR3 was hit: gfn is new CR3 > value, gla is previous */ > -#define MEM_EVENT_REASON_CR4 4 /* CR4 was hit: gfn is new CR4 > value, gla is previous */ > -#define MEM_EVENT_REASON_INT3 5 /* int3 was hit: gla/gfn are RIP > */ > -#define MEM_EVENT_REASON_SINGLESTEP 6 /* single step was invoked: > gla/gfn are RIP */ > -#define MEM_EVENT_REASON_MSR 7 /* MSR was hit: gfn is MSR value, > gla is MSR address; > - does NOT honour > HVMPME_onchangeonly */ > +/* Reasons for the vm event request */ > +/* Default case */ > +#define MEM_EVENT_REASON_UNKNOWN 0 > +/* Memory access violation */ > +#define MEM_EVENT_REASON_MEM_ACCESS_VIOLATION 1 > +/* Memory sharing event */ > +#define MEM_EVENT_REASON_MEM_SHARING 2 > +/* Memory paging event */ > +#define MEM_EVENT_REASON_MEM_PAGING 3 > +/* CR0 was updated */ > +#define MEM_EVENT_REASON_CR0 4 > +/* CR3 was updated */ > +#define MEM_EVENT_REASON_CR3 5 > +/* CR4 was updated */ > +#define MEM_EVENT_REASON_CR4 6 > +/* Debug operation executed (int3) */ > +#define MEM_EVENT_REASON_INT3 7 > +/* Single-step (MTF) */ > +#define MEM_EVENT_REASON_SINGLESTEP 8 > +/* An MSR was updated. Does NOT honour HVMPME_onchangeonly */ > +#define MEM_EVENT_REASON_MSR 9 I see you rename these a second time in patch 5 - can't this renaming be reduced to just one? > @@ -97,16 +114,16 @@ struct mem_event_regs_x86 { > uint32_t _pad; > }; > > -typedef struct mem_event_st { > - uint32_t flags; > - uint32_t vcpu_id; > +struct mem_event_regs { > + union { > + struct mem_event_regs_x86 x86; > + }; > +}; No non-standard (C89) features in public headers please. > +struct mem_event_mem_access_data { > uint64_t gfn; > uint64_t offset; > uint64_t gla; /* if gla_valid */ > - > - uint32_t p2mt; > - > uint16_t access_r:1; > uint16_t access_w:1; > uint16_t access_x:1; I also wonder how well thought through the use of bit fields here is. > +struct mem_event_int3_data { > + uint64_t gfn; > + uint64_t gla; > +}; > + > +struct mem_event_singlestep_data { > + uint64_t gfn; > + uint64_t gla; > +}; I may lack some understanding of the interfaces here, but what do gfn and gla have to do with int3 and single-step events? Also, how architecture-neutral is int3 really?! > +struct mem_event_msr_data { > + uint64_t msr; Maybe better uint32_t with another such typed padding slot following? > +typedef struct mem_event_st { > + uint32_t flags; > + uint32_t vcpu_id; > + uint32_t reason; > + > + union { > + struct mem_event_paging_data mem_paging_event; > + struct mem_event_sharing_data mem_sharing_event; > + struct mem_event_mem_access_data mem_access_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; > + }; > > - uint16_t reason; > - struct mem_event_regs_x86 x86_regs; > + struct mem_event_regs regs; > } mem_event_request_t, mem_event_response_t; This structure's layout now differs between 32- and 64-bit, which I'm sure you want to avoid. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |