[Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Thomas Huth]

The QEMU_PACKED is causing a compiler warning/error with GCC 9:

  CC      block/nvme.o
block/nvme.c: In function ‘nvme_create_queue_pair’:
block/nvme.c:209:22: error: taking address of packed member of
 ‘struct <anonymous>’ may result in an unaligned pointer value
 [-Werror=address-of-packed-member]
  209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];

All members of the struct are naturally aligned, so there should
not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
also ensures that there is no padding. Thus simply remove the QEMU_PACKED
here.

Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index b5952c9..6c2ce7d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -82,7 +82,7 @@ typedef volatile struct {
     uint8_t  reserved1[0xec0];
     uint8_t  cmd_set_specfic[0x100];
     uint32_t doorbells[];
-} QEMU_PACKED NVMeRegs;
+} NVMeRegs;
 
 QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Thomas Huth]

The QEMU_PACKED is causing a compiler warning/error with GCC 9:

  CC      block/nvme.o
block/nvme.c: In function ‘nvme_create_queue_pair’:
block/nvme.c:209:22: error: taking address of packed member of
 ‘struct <anonymous>’ may result in an unaligned pointer value
 [-Werror=address-of-packed-member]
  209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];

All members of the struct are naturally aligned, so there should
not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
also ensures that there is no padding. Thus simply remove the QEMU_PACKED
here.

Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index b5952c9..6c2ce7d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -82,7 +82,7 @@ typedef volatile struct {
     uint8_t  reserved1[0xec0];
     uint8_t  cmd_set_specfic[0x100];
     uint32_t doorbells[];
-} QEMU_PACKED NVMeRegs;
+} NVMeRegs;
 
 QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 
-- 
1.8.3.1

Re: [Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Peter Maydell]

On Mon, 25 Feb 2019 at 11:59, Thomas Huth <thuth@redhat.com> wrote:
>
> The QEMU_PACKED is causing a compiler warning/error with GCC 9:
>
>   CC      block/nvme.o
> block/nvme.c: In function ‘nvme_create_queue_pair’:
> block/nvme.c:209:22: error: taking address of packed member of
>  ‘struct <anonymous>’ may result in an unaligned pointer value
>  [-Werror=address-of-packed-member]
>   209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
>
> All members of the struct are naturally aligned, so there should
> not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
> also ensures that there is no padding. Thus simply remove the QEMU_PACKED
> here.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index b5952c9..6c2ce7d 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -82,7 +82,7 @@ typedef volatile struct {
>      uint8_t  reserved1[0xec0];
>      uint8_t  cmd_set_specfic[0x100];
>      uint32_t doorbells[];
> -} QEMU_PACKED NVMeRegs;
> +} NVMeRegs;
>
>  QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Peter Maydell]

On Mon, 25 Feb 2019 at 11:59, Thomas Huth <thuth@redhat.com> wrote:
>
> The QEMU_PACKED is causing a compiler warning/error with GCC 9:
>
>   CC      block/nvme.o
> block/nvme.c: In function ‘nvme_create_queue_pair’:
> block/nvme.c:209:22: error: taking address of packed member of
>  ‘struct <anonymous>’ may result in an unaligned pointer value
>  [-Werror=address-of-packed-member]
>   209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
>
> All members of the struct are naturally aligned, so there should
> not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
> also ensures that there is no padding. Thus simply remove the QEMU_PACKED
> here.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index b5952c9..6c2ce7d 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -82,7 +82,7 @@ typedef volatile struct {
>      uint8_t  reserved1[0xec0];
>      uint8_t  cmd_set_specfic[0x100];
>      uint32_t doorbells[];
> -} QEMU_PACKED NVMeRegs;
> +} NVMeRegs;
>
>  QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Kevin Wolf]

Am 25.02.2019 um 12:59 hat Thomas Huth geschrieben:
> The QEMU_PACKED is causing a compiler warning/error with GCC 9:
> 
>   CC      block/nvme.o
> block/nvme.c: In function ‘nvme_create_queue_pair’:
> block/nvme.c:209:22: error: taking address of packed member of
>  ‘struct <anonymous>’ may result in an unaligned pointer value
>  [-Werror=address-of-packed-member]
>   209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
> 
> All members of the struct are naturally aligned, so there should
> not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
> also ensures that there is no padding. Thus simply remove the QEMU_PACKED
> here.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks, applied to the block branch.

Though I'm not sure why a compiler should warn if packed and non-packed
result in the exact same layout anyway...

Kevin

Re: [Qemu-devel] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Kevin Wolf]

Am 25.02.2019 um 12:59 hat Thomas Huth geschrieben:
> The QEMU_PACKED is causing a compiler warning/error with GCC 9:
> 
>   CC      block/nvme.o
> block/nvme.c: In function ‘nvme_create_queue_pair’:
> block/nvme.c:209:22: error: taking address of packed member of
>  ‘struct <anonymous>’ may result in an unaligned pointer value
>  [-Werror=address-of-packed-member]
>   209 |     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
> 
> All members of the struct are naturally aligned, so there should
> not be the need for QEMU_PACKED here, and the following QEMU_BUILD_BUG_ON
> also ensures that there is no padding. Thus simply remove the QEMU_PACKED
> here.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817525
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks, applied to the block branch.

Though I'm not sure why a compiler should warn if packed and non-packed
result in the exact same layout anyway...

Kevin

Re: [Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Peter Maydell]

On Mon, 25 Feb 2019 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
> Though I'm not sure why a compiler should warn if packed and non-packed
> result in the exact same layout anyway...

Packed implies "and any time you see a pointer to this struct
it might not be aligned". Non-packed implies "you can assume
that the base of the struct is aligned".

thanks
-- PMM

Re: [Qemu-devel] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Peter Maydell]

On Mon, 25 Feb 2019 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
> Though I'm not sure why a compiler should warn if packed and non-packed
> result in the exact same layout anyway...

Packed implies "and any time you see a pointer to this struct
it might not be aligned". Non-packed implies "you can assume
that the base of the struct is aligned".

thanks
-- PMM

Re: [Qemu-trivial] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Thomas Huth]

On 25/02/2019 15.30, Peter Maydell wrote:
> On Mon, 25 Feb 2019 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
>> Though I'm not sure why a compiler should warn if packed and non-packed
>> result in the exact same layout anyway...
> 
> Packed implies "and any time you see a pointer to this struct
> it might not be aligned". Non-packed implies "you can assume
> that the base of the struct is aligned".

Right. FWIW, consider this small program:

#include <stdio.h>
#include <stdint.h>

typedef struct {
    uint64_t a;
    uint64_t b;
    uint64_t c;
    uint64_t d;
} __attribute__((packed)) s1_t;

struct {
    char x;
    s1_t s1;
} s2;

int main()
{
    printf("address of s2.s1.d = %p\n", &s2.s1.d);
    return 0;
}

Though all members of s1_t look like they are naturally aligned, I get
an odd pointer for one of its members in this case.

So passing along that pointer to other functions will certainly cause
crashes on architectures like Sparc, thus the compiler warning is indeed
justified here.

 Thomas

Re: [Qemu-devel] [PATCH] block/nvme: Remove QEMU_PACKED from naturally aligned NVMeRegs struct

[Thomas Huth]

On 25/02/2019 15.30, Peter Maydell wrote:
> On Mon, 25 Feb 2019 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
>> Though I'm not sure why a compiler should warn if packed and non-packed
>> result in the exact same layout anyway...
> 
> Packed implies "and any time you see a pointer to this struct
> it might not be aligned". Non-packed implies "you can assume
> that the base of the struct is aligned".

Right. FWIW, consider this small program:

#include <stdio.h>
#include <stdint.h>

typedef struct {
    uint64_t a;
    uint64_t b;
    uint64_t c;
    uint64_t d;
} __attribute__((packed)) s1_t;

struct {
    char x;
    s1_t s1;
} s2;

int main()
{
    printf("address of s2.s1.d = %p\n", &s2.s1.d);
    return 0;
}

Though all members of s1_t look like they are naturally aligned, I get
an odd pointer for one of its members in this case.

So passing along that pointer to other functions will certainly cause
crashes on architectures like Sparc, thus the compiler warning is indeed
justified here.

 Thomas