Re: [Xen-devel] [PATCH V7] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 05/28/2015 07:06 PM, Lengyel, Tamas wrote:
> 
> 
> On Thu, May 28, 2015 at 10:33 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>> wrote:
> 
>     On 05/28/2015 05:12 PM, Lengyel, Tamas wrote:
>     >     diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>     >     index 9d5f9f3..a13195d 100644
>     >     --- a/xen/arch/x86/hvm/event.c
>     >     +++ b/xen/arch/x86/hvm/event.c
>     >     @@ -21,6 +21,7 @@
>     >
>     >      #include <xen/vm_event.h>
>     >      #include <xen/paging.h>
>     >     +#include <asm/monitor.h>
>     >      #include <public/vm_event.h>
>     >
>     >      static void hvm_event_fill_regs(vm_event_request_t *req)
>     >     @@ -88,55 +89,28 @@ static int hvm_event_traps(uint8_t sync,
>     >     vm_event_request_t *req)
>     >          return 1;
>     >      }
>     >
>     >     -static inline
>     >     -void hvm_event_cr(uint32_t reason, unsigned long value,
>     >     -                         unsigned long old, bool_t onchangeonly,
>     >     bool_t sync)
>     >     +void (hvm_event_cr)(unsigned int index, unsigned long value,
>     >     unsigned long old)
>     >
>     >
>     > Any reason to have parenthesis around hvm_event_cr? Also, if you could
>     > make the patch available somewhere via git, that would be great, I would
>     > like to test it!
> 
>     Yes, to prevent macro expansion. Jan has suggested that I add a macro to
>     make the calls look neater, please see this change:
> 
>     +void hvm_event_cr(unsigned int index, unsigned long value, unsigned
>     long old);
>     +#define hvm_event_cr(what, new, old) hvm_event_cr(VM_EVENT_X86_##what,
>     new, old)
> 
>     So, unless that call is put in parenthesis, the code you mention is
>     expanded to hvm_event_cr(VM_EVENT_X86_unsigned int index, ...) or
>     something of that nature, and it won't compile.
> 
> 
> That seems a bit odd to me that we now have both a macro and a function
> with the same name. I think making the macro at least all capital would
> avoid confusion and make the code a lot more readable. But even then
> IMHO this wrapper is not helping code readability much. Its really a
> pain to track down auto-expanding fields in macros when someone is
> trying to read the code.

I think the lowercase version of the wrapper macro of the same name is a
Xen coding style convention, but if Jan thinks going uppercase is not a
problem that's fine with me.

As for the wrapper, being right next to the function it is wrapping
makes it hard to ignore when reading the header, and the #define is
pretty clear on what it does.

Also, while I think that the macro does help with readability (so my
preference would be to leave it in), it's just that - a preference - so
if Jan also agrees that we should now remove it I'll do that. After all,
the macro will probably go out the window (or the first parameter will
need to be changed to VM_EVENT_##what instead of VM_EVENT_X86_##what) as
soon as ARM control register write events will come into play.

>     As for a git repo, unfortunately I don't have a public one, but I intend
>     to submit V8 shortly, and if I understood things correctly it's quite
>     likely to enter staging, so it should only be a short while now.
> 
> 
> I guess but putting it up on github as well should really not be a problem..

True, I'll see what I can do about that in the future, but in the
meantime I'll send you the patch file privately (exported with "git
format-patch master") and you can just apply it with "git am" to the
current Xen source code.


Thanks,
Razvan


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