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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.15 at 16:34, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> 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/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.

But without the caller passing you the version of the ABI it uses,
how do you want to do such compatibility handling?

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

Afaics MEM_EVENT_REASON_VIOLATION became
MEM_EVENT_REASON_MEM_ACCESS_VIOLATION,
MEM_EVENT_REASON_MEM_{SHARING,PAGING} got
introduced, and many other got renumbered. And from a
trivial perspective - if what you said was true, the diff could
have retained every other line (as you then would have to
insert further blanks either).

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

But the union has no field name.

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

The same can be achieved with a flags field and a set of constants.

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

For the one at hand - why can't it be named software_breakpoint
or some such?

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

Architecturally there are no MSRs > 0xffffffff.

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

No, add explicit padding.

Jan

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