[PATCH] kvmtrace: fix "Remove use of bit fields in kvm trace structure" patch

[PATCH] kvmtrace: fix "Remove use of bit fields in kvm trace structure" patch

[Christian Ehrhardt]

[PATCH] kvmppc: kvmtrace: fix "Remove use of bit fields in kvm trace 
structure"

From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

The current patch kvm_trace_nobitfields.diff in the kvmppc.hg-dev repo
doesn't compile and has some minor flaws.
This patch is known to the kvm list as "[PATCH] [v2] Remove use of bit
fields in kvm trace structure" from Jerone.

Problem:
code:
   rec.rec_val     |= TRACE_REC_EVENT_ID(va_arg(*args, u32));
makro:
   #define TRACE_REC_EVENT_ID (val) \
                (0x0fffffff & (val))

is extended to:
   rec.rec_val |= (val) (0x0fffffff & (val))(__builtin_va_arg(*args,u32));

that is failing with:
   kvm_trace.c: In function 'kvm_add_trace':
   kvm_trace.c:66: error: 'val' undeclared (first use in this function)
   kvm_trace.c:66: error: (Each undeclared identifier is reported only once
   kvm_trace.c:66: error: for each function it appears in.)

Obviously caused by the blank between macro definition and (val) which makes
it a non-parameter macro.

Additionally the two macro definitions below don't put () around the 
parameter
which is one of the golden macro rules to keep operator precedence 
independent
to what someone passes as argument to your macro.

Further I found that part of the path added a superfluous assignment in
kvm_trace.c (initialize with =0 and directly or something in with |=).

This patch fixes all three issues. As Hollis pointed out yesterday patch 
#1 of
the initial series of 3 patches should already be applied according to your
(Avi) response while it is not seen on kvm-commits yet.
He offered to collect those patches in his patch queue and send you a batch
soon.
That said you can consider this patch FYI (to collect comments now) and wait
until our consolidated batch arrives including all fixes at once.

Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 include/linux/kvm.h  |   10 +++++-----
 virt/kvm/kvm_trace.c |    4 +---
 2 files changed, 6 insertions(+), 8 deletions(-)

[diff]

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -331,12 +331,12 @@
     } u;
 } __attribute__((packed));
 
-#define TRACE_REC_EVENT_ID (val) \
+#define TRACE_REC_EVENT_ID(val) \
         (0x0fffffff & (val))
-#define TRACE_REC_NUM_DATA_ARGS (val) \
-        (0x70000000 & (val << 28))
-#define TRACE_REC_TCS (val) \
-        (0x80000000 & (val << 31))
+#define TRACE_REC_NUM_DATA_ARGS(val) \
+        (0x70000000 & ((val) << 28))
+#define TRACE_REC_TCS(val) \
+        (0x80000000 & ((val) << 31))
 
 #define KVMIO 0xAE
 
diff --git a/virt/kvm/kvm_trace.c b/virt/kvm/kvm_trace.c
--- a/virt/kvm/kvm_trace.c
+++ b/virt/kvm/kvm_trace.c
@@ -60,10 +60,8 @@
     if (unlikely(kt->trace_state != KVM_TRACE_STATE_RUNNING))
         return;
 
-    rec.rec_val = 0;
-   
     /* set event id */   
-    rec.rec_val    |= TRACE_REC_EVENT_ID(va_arg(*args, u32));
+    rec.rec_val    = TRACE_REC_EVENT_ID(va_arg(*args, u32));
 
     vcpu        = va_arg(*args, struct kvm_vcpu *);
     rec.pid        = current->tgid;

---

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization