| 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 :)
Fix a compiler optimization problem introduced in commit e5ff22ba9fc ("block/nvme: Pair doorbell registers"), use atomic operations. For some not understood yet reason using atomic_and triggers a NMI on x86 arch. Using the following snippet on top of this series: -- >8 -- - atomic_set(&s->regs->ctrl.cc, - cpu_to_le32(atomic_read(&s->regs->ctrl.cc) & const_le32(0xFE)= )); + atomic_and(&s->regs->ctrl.cc, const_le32(0xFE)); --- triggers: {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source:= 3 {1}[Hardware Error]: event severity: fatal {1}[Hardware Error]: Error 0, type: fatal {1}[Hardware Error]: section_type: PCIe error {1}[Hardware Error]: port_type: 0, PCIe end point {1}[Hardware Error]: version: 1.16 {1}[Hardware Error]: command: 0x0006, status: 0x0010 {1}[Hardware Error]: device_id: 0000:04:00.0 {1}[Hardware Error]: slot: 0 {1}[Hardware Error]: secondary_bus: 0x00 {1}[Hardware Error]: vendor_id: 0x8086, device_id: 0x2701 {1}[Hardware Error]: class_code: 010802 {1}[Hardware Error]: aer_uncor_status: 0x00100000, aer_uncor_mask: 0x00018= 000 {1}[Hardware Error]: aer_uncor_severity: 0x000e7030 {1}[Hardware Error]: TLP Header: 33000000 00000000 00000000 00000000 Kernel panic - not syncing: Fatal hardware error! CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.8-100.fc30.x86_64 #1 Hardware name: Dell Inc. PowerEdge R430/03XKDV, BIOS 1.2.6 06/08/2015 Call Trace: <NMI> dump_stack+0x66/0x90 panic+0xf1/0x2d3 __ghes_panic.part.0+0x26/0x26 ghes_notify_nmi.cold+0x5/0x5 nmi_handle+0x66/0x120 default_do_nmi+0x45/0x100 do_nmi+0x165/0x1d0 end_repeat_nmi+0x16/0x50 RIP: 0010:intel_idle+0x82/0x130 Code: 65 48 8b 04 25 c0 8b 01 00 0f 01 c8 48 8b 00 a8 08 75 17 e9 07 00 00 0= 0 0f 00 2d f5 cd 6d 00 b9 01 00 00 00 48 89 d8 0f 01 c9 <65> 48 8b 04 25 c0 8= b 01 00 f0 80 60 02 df f0 83 44 24 fc 00 b RSP: 0018:ffffffffa8603e30 EFLAGS: 00000046 RAX: 0000000000000001 RBX: 0000000000000001 RCX: 0000000000000001 RDX: 0000000000000000 RSI: ffffffffa875edc0 RDI: 0000000000000000 RBP: ffffffffa875edc0 R08: 00000013ef2b797f R09: 000000000000ba42 R10: 00000000000993b3 R11: ffffa070afc29ca4 R12: 0000000000000002 R13: ffffffffa875edc0 R14: 0000000000000002 R15: ffffffffa8614840 ? intel_idle+0x82/0x130 ? intel_idle+0x82/0x130 </NMI> cpuidle_enter_state+0x81/0x3e0 cpuidle_enter+0x29/0x40 do_idle+0x1c0/0x260 cpu_startup_entry+0x19/0x20 start_kernel+0x7ad/0x7ba secondary_startup_64+0xb6/0xc0 Kernel Offset: 0x26000000 from 0xffffffff81000000 (relocation range: 0xfffff= fff80000000-0xffffffffbfffffff) Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Philippe Mathieu-Daud=C3=A9 (3): block/nvme: Initialize constant values with const_le32() block/nvme: Use atomic operations instead of 'volatile' keyword block/nvme: Align NVMeRegs structure to 4KiB and mark it packed block/nvme.c | 55 ++++++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 25 deletions(-) --=20 2.26.2