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

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Thu, May 28, 2015 at 10:33 AM, Razvan Cojocaru <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.

Â

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..
Â

Thanks,
Razvan

--

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email Âtlengyel@novetta.com

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