[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 17.11.14 at 17:43, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 11/17/2014 06:37 PM, Jan Beulich wrote:
>>>>> On 12.11.14 at 16:48, <andrew.cooper3@xxxxxxxxxx> 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?

Yes, afaic. But tool stack and mm maintainers need to be on the same
page before you should take this as the final route to follow.

Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.