| Message ID | 1448900541-19939-2-git-send-email-asmetanin@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 30/11/2015 17:22, Andrey Smetanin wrote: > enum hv_message_type inside struct hv_message, hv_post_message > is not size portable. Replace enum by u32. It's only non-portable inside structs. Okay to apply just these: @@ -172,7 +174,7 @@ union hv_message_flags { /* Define synthetic interrupt controller message header. */ struct hv_message_header { - u32 message_type; + enum hv_message_type message_type; u8 payload_size; union hv_message_flags message_flags; u8 reserved[2]; @@ -345,7 +347,7 @@ enum hv_call_code { struct hv_input_post_message { union hv_connection_id connectionid; u32 reserved; - u32 message_type; + enum hv_message_type message_type; u32 payload_size; u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; }; ? Paolo
On 12/02/2015 03:22 PM, Paolo Bonzini wrote: > > On 30/11/2015 17:22, Andrey Smetanin wrote: >> enum hv_message_type inside struct hv_message, hv_post_message >> is not size portable. Replace enum by u32. > It's only non-portable inside structs. Okay to apply just these: > > @@ -172,7 +174,7 @@ union hv_message_flags { > > /* Define synthetic interrupt controller message header. */ > struct hv_message_header { > - u32 message_type; > + enum hv_message_type message_type; > u8 payload_size; > union hv_message_flags message_flags; > u8 reserved[2]; > @@ -345,7 +347,7 @@ enum hv_call_code { > struct hv_input_post_message { > union hv_connection_id connectionid; > u32 reserved; > - u32 message_type; > + enum hv_message_type message_type; > u32 payload_size; > u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; > }; > > ? > > Paolo sorry for the delay. Andrey is on vacation till the end of the week. This could be not enough for some compilers as this exact enum could be signed and unsigned depends upon the implementation of the compiler and if it is signed we can face signed/unsigned comparison in ifs. Vitaly, by the way, this code is a bit rotten. The only caller of hv_post_message calls it with message type exactly written as "1", which is not defined in the enum. /* * vmbus_post_msg - Send a msg on the vmbus's message connection */ int vmbus_post_msg(void *buffer, size_t buflen) { ... ret = hv_post_message(conn_id, 1, buffer, buflen); Den
On 04/12/2015 15:33, Denis V. Lunev wrote: > On 12/02/2015 03:22 PM, Paolo Bonzini wrote: >> >> On 30/11/2015 17:22, Andrey Smetanin wrote: >>> enum hv_message_type inside struct hv_message, hv_post_message >>> is not size portable. Replace enum by u32. >> It's only non-portable inside structs. Okay to apply just these: >> >> @@ -172,7 +174,7 @@ union hv_message_flags { >> >> /* Define synthetic interrupt controller message header. */ >> struct hv_message_header { >> - u32 message_type; >> + enum hv_message_type message_type; >> u8 payload_size; >> union hv_message_flags message_flags; >> u8 reserved[2]; >> @@ -345,7 +347,7 @@ enum hv_call_code { >> struct hv_input_post_message { >> union hv_connection_id connectionid; >> u32 reserved; >> - u32 message_type; >> + enum hv_message_type message_type; >> u32 payload_size; >> u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; >> }; >> >> ? >> >> Paolo > sorry for the delay. > > Andrey is on vacation till the end of the week. > > This could be not enough for some compilers as this exact > enum could be signed and unsigned depends upon the > implementation of the compiler and if it is signed we > can face signed/unsigned comparison in ifs. But why is that a problem? The issue is pre-existing anyway; the only one that can cause bugs when moving code to uapi/ (i.e. which means it can be used on non-x86 platforms) is the size of the enum, not the signedness. Paolo > Vitaly, by the way, this code is a bit rotten. The only caller of > hv_post_message calls it with message type exactly written > as "1", which is not defined in the enum. > > /* > * vmbus_post_msg - Send a msg on the vmbus's message connection > */ > int vmbus_post_msg(void *buffer, size_t buflen) > { > ... > ret = hv_post_message(conn_id, 1, buffer, buflen); > > Den
On 12/04/2015 05:41 PM, Paolo Bonzini wrote: > > On 04/12/2015 15:33, Denis V. Lunev wrote: >> On 12/02/2015 03:22 PM, Paolo Bonzini wrote: >>> On 30/11/2015 17:22, Andrey Smetanin wrote: >>>> enum hv_message_type inside struct hv_message, hv_post_message >>>> is not size portable. Replace enum by u32. >>> It's only non-portable inside structs. Okay to apply just these: >>> >>> @@ -172,7 +174,7 @@ union hv_message_flags { >>> >>> /* Define synthetic interrupt controller message header. */ >>> struct hv_message_header { >>> - u32 message_type; >>> + enum hv_message_type message_type; >>> u8 payload_size; >>> union hv_message_flags message_flags; >>> u8 reserved[2]; >>> @@ -345,7 +347,7 @@ enum hv_call_code { >>> struct hv_input_post_message { >>> union hv_connection_id connectionid; >>> u32 reserved; >>> - u32 message_type; >>> + enum hv_message_type message_type; >>> u32 payload_size; >>> u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; >>> }; >>> >>> ? >>> >>> Paolo >> sorry for the delay. >> >> Andrey is on vacation till the end of the week. >> >> This could be not enough for some compilers as this exact >> enum could be signed and unsigned depends upon the >> implementation of the compiler and if it is signed we >> can face signed/unsigned comparison in ifs. > But why is that a problem? The issue is pre-existing anyway; the only > one that can cause bugs when moving code to uapi/ (i.e. which means it > can be used on non-x86 platforms) is the size of the enum, not the > signedness. > > Paolo we are now comparing enum with enum which are the same type. With the change you are proposing we will compare enum with u32 which are different. Original suggestion from Andrey was safe in this respect. Den
On 04/12/2015 17:55, Denis V. Lunev wrote: > On 12/04/2015 05:41 PM, Paolo Bonzini wrote: >> >> On 04/12/2015 15:33, Denis V. Lunev wrote: >>> On 12/02/2015 03:22 PM, Paolo Bonzini wrote: >>>> On 30/11/2015 17:22, Andrey Smetanin wrote: >>>>> enum hv_message_type inside struct hv_message, hv_post_message >>>>> is not size portable. Replace enum by u32. >>>> It's only non-portable inside structs. Okay to apply just these: >>>> >>>> @@ -172,7 +174,7 @@ union hv_message_flags { >>>> >>>> /* Define synthetic interrupt controller message header. */ >>>> struct hv_message_header { >>>> - u32 message_type; >>>> + enum hv_message_type message_type; >>>> u8 payload_size; >>>> union hv_message_flags message_flags; >>>> u8 reserved[2]; >>>> @@ -345,7 +347,7 @@ enum hv_call_code { >>>> struct hv_input_post_message { >>>> union hv_connection_id connectionid; >>>> u32 reserved; >>>> - u32 message_type; >>>> + enum hv_message_type message_type; >>>> u32 payload_size; >>>> u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; >>>> }; >>>> >>>> ? >>>> >>>> Paolo >>> sorry for the delay. >>> >>> Andrey is on vacation till the end of the week. >>> >>> This could be not enough for some compilers as this exact >>> enum could be signed and unsigned depends upon the >>> implementation of the compiler and if it is signed we >>> can face signed/unsigned comparison in ifs. >> But why is that a problem? The issue is pre-existing anyway; the only >> one that can cause bugs when moving code to uapi/ (i.e. which means it >> can be used on non-x86 platforms) is the size of the enum, not the >> signedness. > > we are now comparing enum with enum which are the same type. > With the change you are proposing we will compare enum > with u32 which are different. This is only an issue in C++. > Original suggestion from Andrey was safe in this respect. Sure, but it makes code less clear. Paolo
On 12/04/2015 08:38 PM, Paolo Bonzini wrote: > > On 04/12/2015 17:55, Denis V. Lunev wrote: >> On 12/04/2015 05:41 PM, Paolo Bonzini wrote: >>> On 04/12/2015 15:33, Denis V. Lunev wrote: >>>> On 12/02/2015 03:22 PM, Paolo Bonzini wrote: >>>>> On 30/11/2015 17:22, Andrey Smetanin wrote: >>>>>> enum hv_message_type inside struct hv_message, hv_post_message >>>>>> is not size portable. Replace enum by u32. >>>>> It's only non-portable inside structs. Okay to apply just these: >>>>> >>>>> @@ -172,7 +174,7 @@ union hv_message_flags { >>>>> >>>>> /* Define synthetic interrupt controller message header. */ >>>>> struct hv_message_header { >>>>> - u32 message_type; >>>>> + enum hv_message_type message_type; >>>>> u8 payload_size; >>>>> union hv_message_flags message_flags; >>>>> u8 reserved[2]; >>>>> @@ -345,7 +347,7 @@ enum hv_call_code { >>>>> struct hv_input_post_message { >>>>> union hv_connection_id connectionid; >>>>> u32 reserved; >>>>> - u32 message_type; >>>>> + enum hv_message_type message_type; >>>>> u32 payload_size; >>>>> u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; >>>>> }; >>>>> >>>>> ? >>>>> >>>>> Paolo >>>> sorry for the delay. >>>> >>>> Andrey is on vacation till the end of the week. >>>> >>>> This could be not enough for some compilers as this exact >>>> enum could be signed and unsigned depends upon the >>>> implementation of the compiler and if it is signed we >>>> can face signed/unsigned comparison in ifs. >>> But why is that a problem? The issue is pre-existing anyway; the only >>> one that can cause bugs when moving code to uapi/ (i.e. which means it >>> can be used on non-x86 platforms) is the size of the enum, not the >>> signedness. >> we are now comparing enum with enum which are the same type. >> With the change you are proposing we will compare enum >> with u32 which are different. > This is only an issue in C++. > >> Original suggestion from Andrey was safe in this respect. > Sure, but it makes code less clear. > > Paolo ok, this seems reasonable. Why not to reduce the patch :) We'll send an update on Monday. Are there any other issue with the patchset? Den
> >> we are now comparing enum with enum which are the same type. > >> With the change you are proposing we will compare enum > >> with u32 which are different. > > This is only an issue in C++. > > > >> Original suggestion from Andrey was safe in this respect. > > Sure, but it makes code less clear. > > > > Paolo > > ok, this seems reasonable. Why not to reduce the patch :) > We'll send an update on Monday. > > Are there any other issue with the patchset? No, I can also do the change myself. Check kvm/queue. Paolo
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c index 6341be8..dde7e1c 100644 --- a/drivers/hv/hv.c +++ b/drivers/hv/hv.c @@ -310,8 +310,8 @@ void hv_cleanup(void) * This involves a hypercall. */ int hv_post_message(union hv_connection_id connection_id, - enum hv_message_type message_type, - void *payload, size_t payload_size) + u32 message_type, + void *payload, size_t payload_size) { struct hv_input_post_message *aligned_msg; diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h index 3782636..e46e18c 100644 --- a/drivers/hv/hyperv_vmbus.h +++ b/drivers/hv/hyperv_vmbus.h @@ -75,32 +75,30 @@ enum hv_cpuid_function { #define HV_EVENT_FLAGS_DWORD_COUNT (256 / sizeof(u32)) /* Define hypervisor message types. */ -enum hv_message_type { - HVMSG_NONE = 0x00000000, +#define HVMSG_NONE 0x00000000 - /* Memory access messages. */ - HVMSG_UNMAPPED_GPA = 0x80000000, - HVMSG_GPA_INTERCEPT = 0x80000001, +/* Memory access messages. */ +#define HVMSG_UNMAPPED_GPA 0x80000000 +#define HVMSG_GPA_INTERCEPT 0x80000001 - /* Timer notification messages. */ - HVMSG_TIMER_EXPIRED = 0x80000010, +/* Timer notification messages. */ +#define HVMSG_TIMER_EXPIRED 0x80000010 - /* Error messages. */ - HVMSG_INVALID_VP_REGISTER_VALUE = 0x80000020, - HVMSG_UNRECOVERABLE_EXCEPTION = 0x80000021, - HVMSG_UNSUPPORTED_FEATURE = 0x80000022, +/* Error messages. */ +#define HVMSG_INVALID_VP_REGISTER_VALUE 0x80000020 +#define HVMSG_UNRECOVERABLE_EXCEPTION 0x80000021 +#define HVMSG_UNSUPPORTED_FEATURE 0x80000022 - /* Trace buffer complete messages. */ - HVMSG_EVENTLOG_BUFFERCOMPLETE = 0x80000040, +/* Trace buffer complete messages. */ +#define HVMSG_EVENTLOG_BUFFERCOMPLETE 0x80000040 - /* Platform-specific processor intercept messages. */ - HVMSG_X64_IOPORT_INTERCEPT = 0x80010000, - HVMSG_X64_MSR_INTERCEPT = 0x80010001, - HVMSG_X64_CPUID_INTERCEPT = 0x80010002, - HVMSG_X64_EXCEPTION_INTERCEPT = 0x80010003, - HVMSG_X64_APIC_EOI = 0x80010004, - HVMSG_X64_LEGACY_FP_ERROR = 0x80010005 -}; +/* Platform-specific processor intercept messages. */ +#define HVMSG_X64_IOPORT_INTERCEPT 0x80010000 +#define HVMSG_X64_MSR_INTERCEPT 0x80010001 +#define HVMSG_X64_CPUID_INTERCEPT 0x80010002 +#define HVMSG_X64_EXCEPTION_INTERCEPT 0x80010003 +#define HVMSG_X64_APIC_EOI 0x80010004 +#define HVMSG_X64_LEGACY_FP_ERROR 0x80010005 #define HV_SYNIC_STIMER_COUNT (4) @@ -174,7 +172,7 @@ union hv_message_flags { /* Define synthetic interrupt controller message header. */ struct hv_message_header { - enum hv_message_type message_type; + u32 message_type; u8 payload_size; union hv_message_flags message_flags; u8 reserved[2]; @@ -347,7 +345,7 @@ enum hv_call_code { struct hv_input_post_message { union hv_connection_id connectionid; u32 reserved; - enum hv_message_type message_type; + u32 message_type; u32 payload_size; u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT]; }; @@ -579,8 +577,8 @@ extern int hv_init(void); extern void hv_cleanup(void); extern int hv_post_message(union hv_connection_id connection_id, - enum hv_message_type message_type, - void *payload, size_t payload_size); + u32 message_type, + void *payload, size_t payload_size); extern u16 hv_signal_event(void *con_id);
enum hv_message_type inside struct hv_message, hv_post_message is not size portable. Replace enum by u32. Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com> CC: Gleb Natapov <gleb@kernel.org> CC: Paolo Bonzini <pbonzini@redhat.com> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Haiyang Zhang <haiyangz@microsoft.com> CC: Vitaly Kuznetsov <vkuznets@redhat.com> CC: Roman Kagan <rkagan@virtuozzo.com> CC: Denis V. Lunev <den@openvz.org> CC: qemu-devel@nongnu.org --- drivers/hv/hv.c | 4 ++-- drivers/hv/hyperv_vmbus.h | 48 +++++++++++++++++++++++------------------------ 2 files changed, 25 insertions(+), 27 deletions(-)