Re: [Xen-devel] [RFC PATCH V2 1/8] xen/mem_event: Cleanup of mem_event structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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