On 19/06/17 09:57, Laszlo Ersek wrote: > On 06/18/17 22:23, Michael S. Tsirkin wrote: >> On Sun, Jun 18, 2017 at 09:02:14AM +0100, Mark Cave-Ayland wrote: >>> By exposing FWCfgIoState and FWCfgMemState internals we allow the >>> possibility >>> for the internal MemoryRegion fields to be mapped by name for boards that >>> wish >>> to wire up the fw_cfg device themselves. >>> >>> An additional minor tidy-up is that the FWCfgEntry typedef is moved from the >>> struct definitions in fw_cfg.h to typedefs.h along with the others. > > I think this paragraph should be dropped.
No problem, I'll do that for the follow-up v6 patchset. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >>> --- >>> hw/nvram/fw_cfg.c | 55 ------------------------------------------ >>> include/hw/nvram/fw_cfg.h | 58 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> include/qemu/typedefs.h | 1 + >>> 3 files changed, 59 insertions(+), 55 deletions(-) >>> >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >>> index df99903..00771c9 100644 >>> --- a/hw/nvram/fw_cfg.c >>> +++ b/hw/nvram/fw_cfg.c >>> @@ -40,14 +40,6 @@ >>> #define FW_CFG_NAME "fw_cfg" >>> #define FW_CFG_PATH "/machine/" FW_CFG_NAME >>> >>> -#define TYPE_FW_CFG "fw_cfg" >>> -#define TYPE_FW_CFG_IO "fw_cfg_io" >>> -#define TYPE_FW_CFG_MEM "fw_cfg_mem" >>> - >>> -#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) >>> -#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) >>> -#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) >>> - >>> /* FW_CFG_VERSION bits */ >>> #define FW_CFG_VERSION 0x01 >>> #define FW_CFG_VERSION_DMA 0x02 >>> @@ -61,53 +53,6 @@ >>> >>> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */ >>> >>> -typedef struct FWCfgEntry { >>> - uint32_t len; >>> - bool allow_write; >>> - uint8_t *data; >>> - void *callback_opaque; >>> - FWCfgReadCallback read_callback; >>> -} FWCfgEntry; >> >> This still doesn't seem to do what Laszlo requested which is to keep >> as many types and macros as possible in fw_cfg.c, only put typedefs in >> fw_cfg.h. > > Sort of; what's missing from this version (for me anyway) is that the > internals of FWCfgEntry should remain in the C file, because we never > depend on those fields -- or the size of that structure -- externally. > I'm OK with the rest. > > Mark, can you please squash the following diff into this patch -- this > is what would implement my request (2) in > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg458313.html>: > >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index b0511b9a9d77..b77ea48abb1d 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -46,14 +46,6 @@ typedef struct FWCfgDmaAccess { >> >> typedef void (*FWCfgReadCallback)(void *opaque); >> >> -struct FWCfgEntry { >> - uint32_t len; >> - bool allow_write; >> - uint8_t *data; >> - void *callback_opaque; >> - FWCfgReadCallback read_callback; >> -}; >> - >> struct FWCfgState { >> /*< private >*/ >> SysBusDevice parent_obj; >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 00771c98505c..9b0aaa21a202 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -53,6 +53,14 @@ >> >> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */ >> >> +struct FWCfgEntry { >> + uint32_t len; >> + bool allow_write; >> + uint8_t *data; >> + void *callback_opaque; >> + FWCfgReadCallback read_callback; >> +}; >> + >> #define JPG_FILE 0 >> #define BMP_FILE 1 >> > > As I wrote in the msg linked above, 'FWCfgEntry need not be moved from > the C file to the header file [...] it's fine to leave FWCfgEntry an > incomplete type in "fw_cfg.h"'. Yes, that's totally my fault as I misinterpreted your comment from your previous email - sorry about that. > With the above two changes (commit message and code update) squashed > into patch #5: > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > and also for the series: > > Tested-by: Laszlo Ersek <ler...@redhat.com> > > (I tested the IO mapped variant with OVMF, exercising fw_cfg DMA and > fw_cfg write, and the MMIO mapped variant with ArmVirtQemu (aka > "AAVMF"), exercising fw_cfg DMA.) Fantastic! It's good to know that the changes don't cause any regressions for both DMA operations and MMIO fw_cfg instances. I'll squash your diff into patch 5, update the tags and then re-post a v6 shortly. Assuming that this is the final revision, who is the right person to accept the patchset into their queue for merge upstream? ATB, Mark.