From 4360235edf4bf308c4656bc2702763e960acc006 Mon Sep 17 00:00:00 2001 From: Shiqing Gao Date: Wed, 5 Sep 2018 10:55:29 +0800 Subject: [PATCH] hv: treewide: fix 'Macro parameter not in brackets' Add the brackets for Macro parameter to avoid the unintentional mistakes. A simple example that may cause mistakes: #define minus(x) -x When the following call is made, z = minus(a-b) it becomes: z = -a-b; where "-a - b" is equivalent to "(-a) - b" rather than "- (a - b)", as expected. v2 -> v3: * convert DMAR_WAIT_COMPLETION to inline function * remove the macro PIC_PIN_FOREACH and implement the well-formed for loop in each case * replace __CPP_STRING with STRINGIFY and remove the unused CPP_STRING v1 -> v2: * Remove some changes to function like macro since MISRA-C requires to use inline functions if it is possible. These MACRO brackets violations will be fixed together when fixing other issues related to function like macro. Tracked-On: #861 Signed-off-by: Shiqing Gao --- hypervisor/arch/x86/vtd.c | 66 +++++++++++++++-------- hypervisor/dm/vpic.c | 26 ++++----- hypervisor/include/arch/x86/cpu.h | 18 +++---- hypervisor/include/arch/x86/guest/guest.h | 32 +++++------ hypervisor/include/arch/x86/per_cpu.h | 2 +- hypervisor/include/common/ptdev.h | 4 +- hypervisor/include/debug/logmsg.h | 2 +- hypervisor/include/lib/list.h | 6 +-- hypervisor/include/lib/macros.h | 4 +- hypervisor/include/lib/stdarg.h | 4 +- 10 files changed, 92 insertions(+), 72 deletions(-) diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c index 8ad51b00e..0fadb8f6f 100644 --- a/hypervisor/arch/x86/vtd.c +++ b/hypervisor/arch/x86/vtd.c @@ -85,20 +85,6 @@ dmar_set_bitslice(uint64_t var, uint64_t mask, #define DMAR_OP_TIMEOUT CYCLES_PER_MS -#define DMAR_WAIT_COMPLETION(offset, condition, status) \ - do { \ - /* variable start isn't used when built as release version */ \ - __unused uint64_t start = rdtsc(); \ - while (1) { \ - status = iommu_read32(dmar_uint, offset); \ - if (condition) \ - break; \ - ASSERT(((rdtsc() - start) < CYCLES_PER_MS), \ - "DMAR OP Timeout!"); \ - asm volatile ("pause" ::: "memory"); \ - } \ - } while (0) - enum dmar_cirg_type { DMAR_CIRG_RESERVED = 0, DMAR_CIRG_GLOBAL, @@ -225,6 +211,36 @@ static void iommu_write64(struct dmar_drhd_rt *dmar_uint, uint32_t offset, mmio_write32(temp, HPA2HVA(dmar_uint->drhd->reg_base_addr + offset + 4U)); } +static inline void +dmar_wait_completion(struct dmar_drhd_rt *dmar_uint, uint32_t offset, + uint32_t mask, bool pre_condition, uint32_t *status) +{ + /* variable start isn't used when built as release version */ + __unused uint64_t start = rdtsc(); + bool condition, temp_condition; + + while (1) { + *status = iommu_read32(dmar_uint, offset); + temp_condition = ((*status & mask) == 0U) ? true : false; + + /* + * pre_condition temp_condition | condition + * -----------------------------------|---------- + * true true | true + * true false | false + * false true | false + * false false | true + */ + condition = (temp_condition == pre_condition) ? true : false; + + if (condition) + break; + ASSERT(((rdtsc() - start) < CYCLES_PER_MS), + "DMAR OP Timeout!"); + asm volatile ("pause" ::: "memory"); + } +} + /* flush cache when root table, context table updated */ static void iommu_flush_cache(struct dmar_drhd_rt *dmar_uint, void *p, uint32_t size) @@ -355,8 +371,8 @@ static void dmar_enable_translation(struct dmar_drhd_rt *dmar_uint) iommu_write32(dmar_uint, DMAR_GCMD_REG, dmar_uint->gcmd); /* 32-bit register */ - DMAR_WAIT_COMPLETION(DMAR_GSTS_REG, (status & DMA_GSTS_TES) != 0U, status); - + dmar_wait_completion(dmar_uint, DMAR_GSTS_REG, DMA_GSTS_TES, false, + &status); status = iommu_read32(dmar_uint, DMAR_GSTS_REG); @@ -374,7 +390,9 @@ static void dmar_disable_translation(struct dmar_drhd_rt *dmar_uint) iommu_write32(dmar_uint, DMAR_GCMD_REG, dmar_uint->gcmd); /* 32-bit register */ - DMAR_WAIT_COMPLETION(DMAR_GSTS_REG, (status & DMA_GSTS_TES) == 0U, status); + dmar_wait_completion(dmar_uint, DMAR_GSTS_REG, DMA_GSTS_TES, true, + &status); + IOMMU_UNLOCK(dmar_uint); } @@ -530,7 +548,8 @@ static void dmar_write_buffer_flush(struct dmar_drhd_rt *dmar_uint) dmar_uint->gcmd | DMA_GCMD_WBF); /* read lower 32 bits to check */ - DMAR_WAIT_COMPLETION(DMAR_GSTS_REG, (status & DMA_GSTS_WBFS) == 0U, status); + dmar_wait_completion(dmar_uint, DMAR_GSTS_REG, DMA_GSTS_WBFS, true, + &status); IOMMU_UNLOCK(dmar_uint); } @@ -565,8 +584,8 @@ static void dmar_invalid_context_cache(struct dmar_drhd_rt *dmar_uint, IOMMU_LOCK(dmar_uint); iommu_write64(dmar_uint, DMAR_CCMD_REG, cmd); /* read upper 32bits to check */ - DMAR_WAIT_COMPLETION(DMAR_CCMD_REG + 4U, (status & DMA_CCMD_ICC_32) == 0U, - status); + dmar_wait_completion(dmar_uint, DMAR_CCMD_REG + 4U, DMA_CCMD_ICC_32, + true, &status); IOMMU_UNLOCK(dmar_uint); @@ -615,8 +634,8 @@ static void dmar_invalid_iotlb(struct dmar_drhd_rt *dmar_uint, iommu_write64(dmar_uint, dmar_uint->ecap_iotlb_offset + 8U, cmd); /* read upper 32bits to check */ - DMAR_WAIT_COMPLETION(dmar_uint->ecap_iotlb_offset + 12U, - (status & DMA_IOTLB_IVT_32) == 0U, status); + dmar_wait_completion(dmar_uint, dmar_uint->ecap_iotlb_offset + 12U, + DMA_IOTLB_IVT_32, true, &status); IOMMU_UNLOCK(dmar_uint); if (dma_iotlb_get_iaig_32(status) == 0U) { @@ -651,7 +670,8 @@ static void dmar_set_root_table(struct dmar_drhd_rt *dmar_uint) dmar_uint->gcmd | DMA_GCMD_SRTP); /* 32-bit register */ - DMAR_WAIT_COMPLETION(DMAR_GSTS_REG, (status & DMA_GSTS_RTPS) != 0U, status); + dmar_wait_completion(dmar_uint, DMAR_GSTS_REG, DMA_GSTS_RTPS, false, + &status); IOMMU_UNLOCK(dmar_uint); } diff --git a/hypervisor/dm/vpic.c b/hypervisor/dm/vpic.c index b67f204e1..7438b9a56 100644 --- a/hypervisor/dm/vpic.c +++ b/hypervisor/dm/vpic.c @@ -43,19 +43,10 @@ enum irqstate { IRQSTATE_PULSE }; - #define NR_VPIC_PINS_PER_CHIP 8U #define NR_VPIC_PINS_TOTAL 16U #define VPIC_INVALID_PIN 0xffU -/* - * Loop over all the pins in priority order from highest to lowest. - */ -#define PIC_PIN_FOREACH(pinvar, i8259, tmpvar) \ - for (tmpvar = 0U, pinvar = (i8259->lowprio + 1U) & 0x7U; \ - tmpvar < NR_VPIC_PINS_PER_CHIP; \ - tmpvar++, pinvar = (pinvar + 1U) & 0x7U) - static void vpic_set_pinstate(struct acrn_vpic *vpic, uint8_t pin, bool newstate); static inline bool master_pic(struct acrn_vpic *vpic, struct i8259_reg_state *i8259) @@ -72,7 +63,9 @@ static inline uint8_t vpic_get_highest_isrpin(struct i8259_reg_state *i8259) { uint8_t bit, pin, i; - PIC_PIN_FOREACH(pin, i8259, i) { + pin = (i8259->lowprio + 1U) & 0x7U; + + for (i = 0U; i < NR_VPIC_PINS_PER_CHIP; i++) { bit = (uint8_t)(1U << pin); if ((i8259->service & bit) != 0U) { @@ -81,11 +74,13 @@ static inline uint8_t vpic_get_highest_isrpin(struct i8259_reg_state *i8259) * cleared by a non-specific EOI in Special Mask Mode. */ if ((i8259->smm != 0U) && ((i8259->mask & bit) != 0U)) { + pin = (pin + 1U) & 0x7U; continue; } else { return pin; } } + pin = (pin + 1U) & 0x7U; } return VPIC_INVALID_PIN; @@ -115,7 +110,9 @@ static inline uint8_t vpic_get_highest_irrpin(struct i8259_reg_state *i8259) serviced = 0U; } - PIC_PIN_FOREACH(pin, i8259, tmp) { + pin = (i8259->lowprio + 1U) & 0x7U; + + for (tmp = 0U; tmp < NR_VPIC_PINS_PER_CHIP; tmp++) { bit = (uint8_t)(1U << pin); /* @@ -133,6 +130,8 @@ static inline uint8_t vpic_get_highest_irrpin(struct i8259_reg_state *i8259) if (((i8259->request & bit) != 0) && ((i8259->mask & bit) == 0)) { return pin; } + + pin = (pin + 1U) & 0x7U; } return VPIC_INVALID_PIN; @@ -314,9 +313,10 @@ static int vpic_ocw1(struct acrn_vpic *vpic, struct i8259_reg_state *i8259, uint vpic->vm, val); i8259->mask = val & 0xffU; + pin = (i8259->lowprio + 1U) & 0x7U; /* query and setup if pin/irq is for passthrough device */ - PIC_PIN_FOREACH(pin, i8259, i) { + for (i = 0U; i < NR_VPIC_PINS_PER_CHIP; i++) { bit = (uint8_t)(1U << pin); /* remap for active: interrupt mask -> unmask @@ -329,6 +329,7 @@ static int vpic_ocw1(struct acrn_vpic *vpic, struct i8259_reg_state *i8259, uint * not device, so not need pt remap */ if ((pin == 2U) && master_pic(vpic, i8259)) { + pin = (pin + 1U) & 0x7U; continue; } @@ -337,6 +338,7 @@ static int vpic_ocw1(struct acrn_vpic *vpic, struct i8259_reg_state *i8259, uint ptdev_intx_pin_remap(vpic->vm, virt_pin, PTDEV_VPIN_PIC); } + pin = (pin + 1U) & 0x7U; } return 0; diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h index 304d6b7a6..31cced16a 100644 --- a/hypervisor/include/arch/x86/cpu.h +++ b/hypervisor/include/arch/x86/cpu.h @@ -334,18 +334,18 @@ void stop_cpus(void); void wait_sync_change(uint64_t *sync, uint64_t wake_sync); /* Read control register */ -#define CPU_CR_READ(cr, result_ptr) \ -{ \ - asm volatile ("mov %%" __CPP_STRING(cr) ", %0" \ - : "=r"(*result_ptr)); \ +#define CPU_CR_READ(cr, result_ptr) \ +{ \ + asm volatile ("mov %%" STRINGIFY(cr) ", %0" \ + : "=r"(*(result_ptr))); \ } /* Write control register */ -#define CPU_CR_WRITE(cr, value) \ -{ \ - asm volatile ("mov %0, %%" __CPP_STRING(cr) \ - : /* No output */ \ - : "r"(value)); \ +#define CPU_CR_WRITE(cr, value) \ +{ \ + asm volatile ("mov %0, %%" STRINGIFY(cr) \ + : /* No output */ \ + : "r"(value)); \ } /* Read MSR */ diff --git a/hypervisor/include/arch/x86/guest/guest.h b/hypervisor/include/arch/x86/guest/guest.h index 3afc72141..c2fea1e94 100644 --- a/hypervisor/include/arch/x86/guest/guest.h +++ b/hypervisor/include/arch/x86/guest/guest.h @@ -18,10 +18,10 @@ #include -#define foreach_vcpu(idx, vm, vcpu) \ - for (idx = 0U, vcpu = vm->hw.vcpu_array[idx]; \ - idx < vm->hw.num_vcpus; \ - idx++, vcpu = vm->hw.vcpu_array[idx]) \ +#define foreach_vcpu(idx, vm, vcpu) \ + for ((idx) = 0U, vcpu = vm->hw.vcpu_array[(idx)]; \ + (idx) < vm->hw.num_vcpus; \ + (idx)++, vcpu = vm->hw.vcpu_array[(idx)]) \ if (vcpu != NULL) /* the index is matched with emulated msrs array*/ @@ -46,20 +46,20 @@ #define E820_MAX_ENTRIES 32U -#define save_segment(seg, SEG_NAME) \ -{ \ - seg.selector = exec_vmread16(SEG_NAME##_SEL); \ - seg.base = exec_vmread(SEG_NAME##_BASE); \ - seg.limit = exec_vmread32(SEG_NAME##_LIMIT); \ - seg.attr = exec_vmread32(SEG_NAME##_ATTR); \ +#define save_segment(seg, SEG_NAME) \ +{ \ + (seg).selector = exec_vmread16(SEG_NAME##_SEL); \ + (seg).base = exec_vmread(SEG_NAME##_BASE); \ + (seg).limit = exec_vmread32(SEG_NAME##_LIMIT); \ + (seg).attr = exec_vmread32(SEG_NAME##_ATTR); \ } -#define load_segment(seg, SEG_NAME) \ -{ \ - exec_vmwrite16(SEG_NAME##_SEL, seg.selector); \ - exec_vmwrite(SEG_NAME##_BASE, seg.base); \ - exec_vmwrite32(SEG_NAME##_LIMIT, seg.limit); \ - exec_vmwrite32(SEG_NAME##_ATTR, seg.attr); \ +#define load_segment(seg, SEG_NAME) \ +{ \ + exec_vmwrite16(SEG_NAME##_SEL, (seg).selector); \ + exec_vmwrite(SEG_NAME##_BASE, (seg).base); \ + exec_vmwrite32(SEG_NAME##_LIMIT, (seg).limit); \ + exec_vmwrite32(SEG_NAME##_ATTR, (seg).attr); \ } struct e820_mem_params { diff --git a/hypervisor/include/arch/x86/per_cpu.h b/hypervisor/include/arch/x86/per_cpu.h index a7fd32ae4..605bd3ebf 100644 --- a/hypervisor/include/arch/x86/per_cpu.h +++ b/hypervisor/include/arch/x86/per_cpu.h @@ -57,7 +57,7 @@ extern uint64_t pcpu_active_bitmap; * get percpu data for pcpu_id. */ #define per_cpu(name, pcpu_id) \ - (per_cpu_data_base_ptr[pcpu_id].name) + (per_cpu_data_base_ptr[(pcpu_id)].name) /* get percpu data for current pcpu */ #define get_cpu_var(name) per_cpu(name, get_cpu_id()) diff --git a/hypervisor/include/common/ptdev.h b/hypervisor/include/common/ptdev.h index b4f70b7bf..a9f4a70db 100644 --- a/hypervisor/include/common/ptdev.h +++ b/hypervisor/include/common/ptdev.h @@ -18,10 +18,10 @@ enum ptdev_vpin_source { }; #define DEFINE_MSI_SID(name, a, b) \ -union source_id name = {.msi_id = {.bdf = (a), .entry_nr = (b)}} +union source_id (name) = {.msi_id = {.bdf = (a), .entry_nr = (b)} } #define DEFINE_IOAPIC_SID(name, a, b) \ -union source_id name = {.intx_id = {.pin = (a), .src = (b)}} +union source_id (name) = {.intx_id = {.pin = (a), .src = (b)} } union source_id { uint32_t value; diff --git a/hypervisor/include/debug/logmsg.h b/hypervisor/include/debug/logmsg.h index 51fad560e..513e08fb1 100644 --- a/hypervisor/include/debug/logmsg.h +++ b/hypervisor/include/debug/logmsg.h @@ -134,7 +134,7 @@ static inline int vprintf(__unused const char *fmt, __unused va_list args) #define dev_dbg(lvl, ...) \ do { \ - do_logmsg(lvl, pr_prefix __VA_ARGS__); \ + do_logmsg((lvl), pr_prefix __VA_ARGS__); \ } while (0) #define panic(...) \ diff --git a/hypervisor/include/lib/list.h b/hypervisor/include/lib/list.h index 45b7cb5ea..1afa532c2 100644 --- a/hypervisor/include/lib/list.h +++ b/hypervisor/include/lib/list.h @@ -112,11 +112,11 @@ static inline void list_splice_init(struct list_head *list, ((type *)((char *)(ptr)-(uint64_t)(&((type *)0)->member))) #define list_for_each(pos, head) \ - for (pos = (head)->next; pos != (head); pos = pos->next) + for ((pos) = (head)->next; (pos) != (head); (pos) = (pos)->next) #define list_for_each_safe(pos, n, head) \ - for (pos = (head)->next, n = pos->next; pos != (head); \ - pos = n, n = pos->next) + for ((pos) = (head)->next, (n) = (pos)->next; (pos) != (head); \ + (pos) = (n), (n) = (pos)->next) #define get_first_item(attached, type, member) \ ((type *)((char *)((attached)->next)-(uint64_t)(&((type *)0)->member))) diff --git a/hypervisor/include/lib/macros.h b/hypervisor/include/lib/macros.h index 7899c2387..f0abd7fd5 100644 --- a/hypervisor/include/lib/macros.h +++ b/hypervisor/include/lib/macros.h @@ -8,9 +8,7 @@ #define MACROS_H /** Replaces 'x' by the string "x". */ -#define __CPP_STRING(x) #x -/** Replaces 'x' by its value. */ -#define CPP_STRING(x) __CPP_STRING(x) +#define STRINGIFY(x) #x /* Macro used to check if a value is aligned to the required boundary. * Returns TRUE if aligned; FALSE if not aligned diff --git a/hypervisor/include/lib/stdarg.h b/hypervisor/include/lib/stdarg.h index 183da216a..fee02c449 100644 --- a/hypervisor/include/lib/stdarg.h +++ b/hypervisor/include/lib/stdarg.h @@ -9,7 +9,7 @@ #include -#define va_start(x, y) __builtin_va_start((x), (y)) -#define va_end(x) __builtin_va_end(x) +#define va_start __builtin_va_start +#define va_end __builtin_va_end #endif /* STDARG_H */