|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH V2 1/8] xen/mem_event: Cleanup of mem_event structures
On Thu, Jan 22, 2015 at 5:00 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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?
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.
>
>>>> @@ -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).
Ah yes, sorry for the noise. I was only looking at the last couple
defines and missed that we actually changed the others. The renaming
certainly can be done later, but the introduction of the new defines
needs to happen here.
>
>>>> @@ -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.
I see. I'll just move this union into mem_event_st and name it regs there.
>
>>>> +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.
>
>>>> +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?
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.
>
>>>> +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.
Ack.
>
>>>> +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. Wouldn't making the
struct packed just take care of it automatically?
>
> Jan
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |