Re: [PATCH V2 1/4] x86/kvm: Resolve some missing-initializers warnings

Rustad, Mark D
2014-07-31T16:35:50Z

On Jul 31, 2014, at 4:50 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 30/07/2014 23:18, Mark D Rustad ha scritto:
>> Resolve some missing-initializers warnings that appear in W=2
>> builds. They are resolved by adding the name as a parameter to
>> the macros and having the macro generate all four fields of the
>> structure.
>>
>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>
>> ---
>> V2: Change macro to supply all four fields instead of using a
>> designated initializer. Also fix up the array terminator.
>> ---
>> arch/x86/kvm/x86.c | 71 ++++++++++++++++++++++++++--------------------------
>> 1 file changed, 36 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index ef432f891d30..623aea52ceba 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -82,8 +82,9 @@ u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
>> static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
>> #endif
>>
>> -#define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
>> -#define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
>> +#define VM_STAT(name, x) name, offsetof(struct kvm, stat.x), KVM_STAT_VM, NULL
>> +#define VCPU_STAT(name, x) name, offsetof(struct kvm_vcpu, stat.x), \
>> + KVM_STAT_VCPU, NULL
>>
>> static void update_cr8_intercept(struct kvm_vcpu *vcpu);
>> static void process_nmi(struct kvm_vcpu *vcpu);
>> @@ -128,39 +129,39 @@ static struct kvm_shared_msrs_global __read_mostly shared_msrs_global;
>> static struct kvm_shared_msrs __percpu *shared_msrs;
>>
>> struct kvm_stats_debugfs_item debugfs_entries[] = {
>> - { "pf_fixed", VCPU_STAT(pf_fixed) },
>> - { "pf_guest", VCPU_STAT(pf_guest) },
>> - { "tlb_flush", VCPU_STAT(tlb_flush) },
>> - { "invlpg", VCPU_STAT(invlpg) },
>> - { "exits", VCPU_STAT(exits) },
>> - { "io_exits", VCPU_STAT(io_exits) },
>> - { "mmio_exits", VCPU_STAT(mmio_exits) },
>> - { "signal_exits", VCPU_STAT(signal_exits) },
>> - { "irq_window", VCPU_STAT(irq_window_exits) },
>> - { "nmi_window", VCPU_STAT(nmi_window_exits) },
>> - { "halt_exits", VCPU_STAT(halt_exits) },
>> - { "halt_wakeup", VCPU_STAT(halt_wakeup) },
>> - { "hypercalls", VCPU_STAT(hypercalls) },
>> - { "request_irq", VCPU_STAT(request_irq_exits) },
>> - { "irq_exits", VCPU_STAT(irq_exits) },
>> - { "host_state_reload", VCPU_STAT(host_state_reload) },
>> - { "efer_reload", VCPU_STAT(efer_reload) },
>> - { "fpu_reload", VCPU_STAT(fpu_reload) },
>> - { "insn_emulation", VCPU_STAT(insn_emulation) },
>> - { "insn_emulation_fail", VCPU_STAT(insn_emulation_fail) },
>> - { "irq_injections", VCPU_STAT(irq_injections) },
>> - { "nmi_injections", VCPU_STAT(nmi_injections) },
>> - { "mmu_shadow_zapped", VM_STAT(mmu_shadow_zapped) },
>> - { "mmu_pte_write", VM_STAT(mmu_pte_write) },
>> - { "mmu_pte_updated", VM_STAT(mmu_pte_updated) },
>> - { "mmu_pde_zapped", VM_STAT(mmu_pde_zapped) },
>> - { "mmu_flooded", VM_STAT(mmu_flooded) },
>> - { "mmu_recycled", VM_STAT(mmu_recycled) },
>> - { "mmu_cache_miss", VM_STAT(mmu_cache_miss) },
>> - { "mmu_unsync", VM_STAT(mmu_unsync) },
>> - { "remote_tlb_flush", VM_STAT(remote_tlb_flush) },
>> - { "largepages", VM_STAT(lpages) },
>> - { NULL }
>> + { VCPU_STAT("pf_fixed", pf_fixed) },
>> + { VCPU_STAT("pf_guest", pf_guest) },
>> + { VCPU_STAT("tlb_flush", tlb_flush) },
>> + { VCPU_STAT("invlpg", invlpg) },
>> + { VCPU_STAT("exits", exits) },
>> + { VCPU_STAT("io_exits", io_exits) },
>> + { VCPU_STAT("mmio_exits", mmio_exits) },
>> + { VCPU_STAT("signal_exits", signal_exits) },
>> + { VCPU_STAT("irq_window", irq_window_exits) },
>> + { VCPU_STAT("nmi_window", nmi_window_exits) },
>> + { VCPU_STAT("halt_exits", halt_exits) },
>> + { VCPU_STAT("halt_wakeup", halt_wakeup) },
>> + { VCPU_STAT("hypercalls", hypercalls) },
>> + { VCPU_STAT("request_irq", request_irq_exits) },
>> + { VCPU_STAT("irq_exits", irq_exits) },
>> + { VCPU_STAT("host_state_reload", host_state_reload) },
>> + { VCPU_STAT("efer_reload", efer_reload) },
>> + { VCPU_STAT("fpu_reload", fpu_reload) },
>> + { VCPU_STAT("insn_emulation", insn_emulation) },
>> + { VCPU_STAT("insn_emulation_fail", insn_emulation_fail) },
>> + { VCPU_STAT("irq_injections", irq_injections) },
>> + { VCPU_STAT("nmi_injections", nmi_injections) },
>> + { VM_STAT("mmu_shadow_zapped", mmu_shadow_zapped) },
>> + { VM_STAT("mmu_pte_write", mmu_pte_write) },
>> + { VM_STAT("mmu_pte_updated", mmu_pte_updated) },
>> + { VM_STAT("mmu_pde_zapped", mmu_pde_zapped) },
>> + { VM_STAT("mmu_flooded", mmu_flooded) },
>> + { VM_STAT("mmu_recycled", mmu_recycled) },
>> + { VM_STAT("mmu_cache_miss", mmu_cache_miss) },
>> + { VM_STAT("mmu_unsync", mmu_unsync) },
>> + { VM_STAT("remote_tlb_flush", remote_tlb_flush) },
>> + { VM_STAT("largepages", lpages) },
>
> Please move the braces inside the macro as well.

I should have thought of that. I will do that in a new version. That would be much better.

>> + { NULL, 0, 0, NULL }
>
> This is ugly. Do you really mind having one residual warning? :)

I agree it is ugly. .name = NULL would be enough to silence it. Would that be better? At the moment I am thinking of this as a test case for the other 1000 { } and {0} initializers in the kernel that are throwing warnings. I know we both agree that the compiler really shouldn't be warning on them, but they currently make a lot noise.

How would you feel about a macro called something like ZERO_ENTRY defined something like:

#define ZERO_ENTRY DIAG_PUSH DIAG_IGNORE(missing-field-initializers) { } DIAG_POP

Where the DIAG_ macros pretty much do what you think. I have another patch series that Jeff hasn't gotten to yet that defines such macros. Of course they get put to good use.

At this point, I'll put the terminator back the way it was, but I would still like your opinion on the macro approach to addressing all of these terminators.

--
Mark Rustad, Networking Division, Intel Corporation