[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg
From: |
Mark Cave-Ayland |
Subject: |
Re: [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h |
Date: |
Sun, 18 Jun 2017 09:01:51 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 16/06/17 22:28, Laszlo Ersek wrote:
> This patch is generally good, but I'd like to suggest improvements:
>
> (1) In the commit message, please mention that we are exposing the
> internals of FWCfgIoState and FWCfgMemState so that board code can map
> the MemoryRegion fields (such as comb_iomem) *by name*.
>
> (2) FWCfgEntry need not be moved from the C file to the header file,
> since we never depend on the *size* of that structure. Instead, please
> add a single line to "include/qemu/typedefs.h":
>
> typedef struct FWCfgEntry FWCfgEntry;
>
> and modify
>
> typedef struct FWCfgEntry {
> ...
> } FWCfgEntry;
>
> in "fw_cfg.c" to just
>
> struct FWCfgEntry {
> ...
> };
>
> Then "fw_cfg.h" can simply include "typedefs.h" and say
>
> ...
> FWCfgEntry *entries[2];
> ...
>
> IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".
>
> (3) When you fix up patch #1 like I requested, removing the
> "FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
> forget to update this patch as well, so that you not re-introduce those
> fields in the header file.
>
> ... I'm positively satisfied with this series (I plan to "grant" my R-b
> for PATCH v5 5/5, with the above updates), but given that I'm no QOM
> expert by any means, I'd like someone with more QOM experience to review
> the patchset as well.
Thanks for the feedback! I've just revised the patchset based upon your
comments and will post the v5 to the list shortly.
ATB,
Mark.
- [Qemu-devel] [PATCHv4 0/5] fw_cfg: qdev-related tidy-ups, Mark Cave-Ayland, 2017/06/16
- [Qemu-devel] [PATCHv4 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Mark Cave-Ayland, 2017/06/16
- [Qemu-devel] [PATCHv4 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1(), Mark Cave-Ayland, 2017/06/16
- [Qemu-devel] [PATCHv4 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize(), Mark Cave-Ayland, 2017/06/16
- [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h, Mark Cave-Ayland, 2017/06/16
- [Qemu-devel] [PATCHv4 4/5] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/06/16