Re: [PATCH 3/3] block/nvme: Align NVMeRegs structure to 4KiB and mark it packed

[Stefan Hajnoczi]

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?

signature.asc
Description: PGP signature