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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.01.15 at 17:23, <tamas.lengyel@zentific.com> wrote:
> On Thu, Jan 22, 2015 at 5:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.01.15 at 16:34, <tamas.lengyel@zentific.com> wrote:
>>> On Thu, Jan 22, 2015 at 4:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 18.01.15 at 16:17, <tamas.lengyel@zentific.com> 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?
> 
> True. I was more imagining this flag to be used by the user to
> determine (and know) what Xen supports. Currently we have to deduce
> that by checking for various functions and structures being defined.
> This would just simply that process for the user.

If you think this will work going forward... Domctl and sysctl
specifically do it the other way. Plus if you do it the way you
describe, I can't see why __XEN_LATEST_INTERFACE_VERSION__
wouldn't be sufficient.

>>>>> +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.
> 
> True. I prefer having the variables directly describing its values
> instead of having separate defines for the bits (like how i did for
> struct npfec). IMHO it easier to read, but that's of course just a
> personal preference.

I too prefer this, but interface definitions require putting aside
such preferences. Just consider what happens on bi-endianness
architectures when consumer and producer use different
endianness. Since the bitfields aren't accessible as an addressable
entity, you can't byte swap them as you would need to. Plus
compilers have (or should I say take) quite a bit more freedom on
what they do to bitfields than what they may do to ordinary
structure members.

>>> 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?
> 
> It can be named that - what would be the benefit? We still don't have
> software breakpoints trappable to hyp mode on ARM so it still will be
> just for x86.

But you could re-use "software_breakpoint" for ARM, while you can't
re-use "int3".

>>>>> +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.
> 
> I was hoping to avoid that as it makes the structure quite messy to
> read and in case a new struct is introduced to be delivered via
> vm_event it may has to adjust the padding again.

How that? Fields with alignment bigger than that of uint64_t aren't
likely to appear.

> Wouldn't making the
> struct packed just take care of it automatically?

How would you envision to do that without using compiler
extensions?

Jan