Re: [Qemu-devel] [PATCHv5 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

["Michael S. Tsirkin" <mst@redhat.com>]

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.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@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.

> -
> -struct FWCfgState {
> -    /*< private >*/
> -    SysBusDevice parent_obj;
> -    /*< public >*/
> -
> -    uint16_t file_slots;
> -    FWCfgEntry *entries[2];
> -    int *entry_order;
> -    FWCfgFiles *files;
> -    uint16_t cur_entry;
> -    uint32_t cur_offset;
> -    Notifier machine_ready;
> -
> -    int fw_cfg_order_override;
> -
> -    bool dma_enabled;
> -    dma_addr_t dma_addr;
> -    AddressSpace *dma_as;
> -    MemoryRegion dma_iomem;
> -};
> -
> -struct FWCfgIoState {
> -    /*< private >*/
> -    FWCfgState parent_obj;
> -    /*< public >*/
> -
> -    MemoryRegion comb_iomem;
> -};
> -
> -struct FWCfgMemState {
> -    /*< private >*/
> -    FWCfgState parent_obj;
> -    /*< public >*/
> -
> -    MemoryRegion ctl_iomem, data_iomem;
> -    uint32_t data_width;
> -    MemoryRegionOps wide_data_ops;
> -};
> -
>  #define JPG_FILE 0
>  #define BMP_FILE 1
>  
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b980cba..b0511b9 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -1,8 +1,19 @@
>  #ifndef FW_CFG_H
>  #define FW_CFG_H
>  
> +#include "qemu/typedefs.h"
>  #include "exec/hwaddr.h"
>  #include "hw/nvram/fw_cfg_keys.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +
> +#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)
>  
>  typedef struct FWCfgFile {
>      uint32_t  size;        /* file size */
> @@ -35,6 +46,53 @@ 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;
> +    /*< public >*/
> +
> +    uint16_t file_slots;
> +    FWCfgEntry *entries[2];
> +    int *entry_order;
> +    FWCfgFiles *files;
> +    uint16_t cur_entry;
> +    uint32_t cur_offset;
> +    Notifier machine_ready;
> +
> +    int fw_cfg_order_override;
> +
> +    bool dma_enabled;
> +    dma_addr_t dma_addr;
> +    AddressSpace *dma_as;
> +    MemoryRegion dma_iomem;
> +};
> +
> +struct FWCfgIoState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion comb_iomem;
> +};
> +
> +struct FWCfgMemState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion ctl_iomem, data_iomem;
> +    uint32_t data_width;
> +    MemoryRegionOps wide_data_ops;
> +};
> +
>  /**
>   * fw_cfg_add_bytes:
>   * @s: fw_cfg device being modified
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index f745d5f..2db2918 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -30,6 +30,7 @@ typedef struct DisplaySurface DisplaySurface;
>  typedef struct DriveInfo DriveInfo;
>  typedef struct Error Error;
>  typedef struct EventNotifier EventNotifier;
> +typedef struct FWCfgEntry FWCfgEntry;
>  typedef struct FWCfgIoState FWCfgIoState;
>  typedef struct FWCfgMemState FWCfgMemState;
>  typedef struct FWCfgState FWCfgState;
> -- 
> 1.7.10.4