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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Jan 22, 2015 at 4:00 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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.

I have to revisit what is actually going on here, I believe I had to
add this to get Xen to build. On a second look not sure why that was.

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

It is intended to be used to establish an API version for backwards
compatibility. We don't have any backwards compatibility checks at
this point, but this will change as soon as we extend this interface
as things go forward in the future.

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

I didn't rename anything here, just updated the comments.

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

Elaborate please? I have this union as we will likely have ARM
registers as well soon. I guess it can wait and be introduced when the
ARM side catches up.

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

It saves some space on the ring. Maybe a uint8_t is enough.

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

These contained auxiliary info about the instruction triggering the
event. It is somewhat superseded at this point by the automatic
filling of the x86 registers struct, however, the gfn is not included
in that struct so it can still be useful.

Also, how
> architecture-neutral is int3 really?!

These aren't architecture neutral by any means. However, we likely are
going to have SMC events on the ARM as well, which won't be
architecture neutral either. I don't see a way around it. But why
would this interface have to be architecture neutral? IMHO a comment
saying these are features only for Intel/AMD/ARM would be sufficient.
We already do checks for the architecture when the user attempts to
enable these features to see if it actually supported on the hardware
or not.

>
>> +struct mem_event_msr_data {
>> +    uint64_t msr;
>
> Maybe better uint32_t with another such typed padding slot following?

Not sure I follow. Could you elaborate what the issue is?

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

Any suggestions? Make the struct(s) packed?

>
> Jan

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel