[Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED

[Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED

[Peter Maydell]

clang complains about taking the address of a packed
member of a struct:

target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
    c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
                          ^~~~~~~~~~~~~~~~
target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
        c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
                              ^~~~~~~~~~~~~~~~

The kernel's definitions of struct kvm_cpuid2 and struct
kvm_cpuid_entry2 are carefully set up with padding fields
so that there is no between-struct padding anyway, so
the QEMU_PACKED annotation is unnecessary and might result
in the compiler generating worse code. Drop it, and instead
assert at build time that there is no stray padding.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/i386/kvm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index b2401d13ea7..739cf8c8ea1 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -864,7 +864,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
     struct {
         struct kvm_cpuid2 cpuid;
         struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
-    } QEMU_PACKED cpuid_data;
+    } cpuid_data;
+    /*
+     * The kernel defines these structs with padding fields so there
+     * should be no extra padding in our cpuid_data struct.
+     */
+    QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
+                      sizeof(struct kvm_cpuid2) +
+                      sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
+
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
     uint32_t limit, i, j, cpuid_i;
-- 
2.19.2

Re: [Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED

[Philippe Mathieu-Daudé]

On 12/10/18 12:46 PM, Peter Maydell wrote:
> clang complains about taking the address of a packed
> member of a struct:
> 
> target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>                           ^~~~~~~~~~~~~~~~
> target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>         c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
>                               ^~~~~~~~~~~~~~~~
> 
> The kernel's definitions of struct kvm_cpuid2 and struct
> kvm_cpuid_entry2 are carefully set up with padding fields
> so that there is no between-struct padding anyway, so
> the QEMU_PACKED annotation is unnecessary and might result
> in the compiler generating worse code. Drop it, and instead
> assert at build time that there is no stray padding.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/i386/kvm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index b2401d13ea7..739cf8c8ea1 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -864,7 +864,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      struct {
>          struct kvm_cpuid2 cpuid;
>          struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> -    } QEMU_PACKED cpuid_data;
> +    } cpuid_data;
> +    /*
> +     * The kernel defines these structs with padding fields so there
> +     * should be no extra padding in our cpuid_data struct.
> +     */
> +    QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
> +                      sizeof(struct kvm_cpuid2) +
> +                      sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
> +
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
>      uint32_t limit, i, j, cpuid_i;
> 

Re: [Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED

[Richard Henderson]

On 12/10/18 5:46 AM, Peter Maydell wrote:
> clang complains about taking the address of a packed
> member of a struct:
> 
> target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>                           ^~~~~~~~~~~~~~~~
> target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>         c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
>                               ^~~~~~~~~~~~~~~~
> 
> The kernel's definitions of struct kvm_cpuid2 and struct
> kvm_cpuid_entry2 are carefully set up with padding fields
> so that there is no between-struct padding anyway, so
> the QEMU_PACKED annotation is unnecessary and might result
> in the compiler generating worse code. Drop it, and instead
> assert at build time that there is no stray padding.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/i386/kvm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED

[Eduardo Habkost]

On Mon, Dec 10, 2018 at 11:46:54AM +0000, Peter Maydell wrote:
> clang complains about taking the address of a packed
> member of a struct:
> 
> target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>                           ^~~~~~~~~~~~~~~~
> target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid' of class or structure '' may result in an unaligned pointer value [-Waddress-of-packed-member]
>         c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
>                               ^~~~~~~~~~~~~~~~
> 
> The kernel's definitions of struct kvm_cpuid2 and struct
> kvm_cpuid_entry2 are carefully set up with padding fields
> so that there is no between-struct padding anyway, so
> the QEMU_PACKED annotation is unnecessary and might result
> in the compiler generating worse code. Drop it, and instead
> assert at build time that there is no stray padding.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Queued, thanks.

-- 
Eduardo