| Message ID | 20200916204004.1511985-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Fix NVMeRegs alignment/packing and use atomic operations | expand |
On Wed, Sep 16, 2020 at 10:40:04PM +0200, Philippe Mathieu-Daudé wrote: > In commit e5ff22ba9fc we changed the doorbells register > declaration but forgot to mark the structure packed (as > MMIO registers), allowing the compiler to optimize it. Does this patch actually change anything? NvmeBar is already 4096 bytes long and struct doorbells has two 32-bit fields, so there is no padding. I ask because it's not clear whether this patch is a bug fix or a cleanup. > /* Memory mapped registers */ > -typedef struct { > +typedef struct QEMU_PACKED { > NvmeBar ctrl; > struct { > uint32_t sq_tail; > uint32_t cq_head; > } doorbells[]; > -} NVMeRegs; > +} QEMU_ALIGNED(4 * KiB) NVMeRegs; I can think of two policies for using packed: 1. Packed is used only when needed to avoid padding. But this patch uses it even though there is no padding, so it can't be this policy. 2. Packed is always used on external binary structs (e.g. file formats, network protocols, hardware register layouts, etc). But doorbells[] isn't marked packed so it can't be this policy either. The documentation says that nested structs need to be marked packed too: https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Common-Type-Attributes.html#Common-Type-Attributes So I'm confused about the purpose of this patch. What does it achieve?
diff --git a/block/nvme.c b/block/nvme.c index be80ea1f410..2f9f560ccd5 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include <linux/vfio.h> #include "qapi/error.h" #include "qapi/qmp/qdict.h" @@ -82,13 +83,15 @@ typedef struct { } NVMeQueuePair; /* Memory mapped registers */ -typedef struct { +typedef struct QEMU_PACKED { NvmeBar ctrl; struct { uint32_t sq_tail; uint32_t cq_head; } doorbells[]; -} NVMeRegs; +} QEMU_ALIGNED(4 * KiB) NVMeRegs; + +QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells[1]) != 4096 + 8); #define INDEX_ADMIN 0 #define INDEX_IO(n) (1 + n)
In commit e5ff22ba9fc we changed the doorbells register declaration but forgot to mark the structure packed (as MMIO registers), allowing the compiler to optimize it. Fix by marking it packed, and align it to avoid: block/nvme.c: In function ‘nvme_create_queue_pair’: block/nvme.c:252:22: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member] 252 | q->sq.doorbell = &s->regs->doorbells[idx * s->doorbell_scale].sq_tail; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fixes: e5ff22ba9fc ("block/nvme: Pair doorbell registers") Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- block/nvme.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)