Re: [PATCH RFC 0/7] xen: Clean-up of mem_event subsystem

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 11/17/2014 06:37 PM, Jan Beulich wrote:
>>>> On 12.11.14 at 16:48, <andrew.cooper3@citrix.com> 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