Message ID | 20200916204004.1511985-1-philmd@redhat.com |
---|---|
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?
On 9/17/20 11:55 AM, Stefan Hajnoczi wrote: > On Wed, Sep 16, 2020 at 10:40:02PM +0200, Philippe Mathieu-Daudé wrote: >> To avoid multiple endianess conversion, as we know the device >> registers are in little-endian, directly use const_le32() with >> constant values. > > Can cpu_to_X() be extended to handle constant expressions? That way the > programmer doesn't need to remember the const_X() API exists. > > Maybe __builtin_constant_p() can be used: > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fconstant_005fp Yes it works! Good idea :)