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

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Jul 14, 2017 at 08:28:37PM +0200, Laszlo Ersek wrote:
> On 07/14/17 20:09, Eduardo Habkost wrote:
> > On Fri, Jul 14, 2017 at 10:40:08AM +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.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > [...]
> >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> >> index b980cba..b77ea48 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,45 @@ typedef struct FWCfgDmaAccess {
> >>  
> >>  typedef void (*FWCfgReadCallback)(void *opaque);
> >>  
> >> +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;
> >> +};
> > 
> > Why do you need the full struct declaration to be exposed in the
> > header?
> 
> Different board code wants to hook up "comb_iomem" manually to different
> address spaces, so they need to access the field directly. This is the
> ultimate goal of the entire exercise, IIRC.
> 
> > The memory regions are supposed to be visible as QOM
> > children to the fw_cfg device, already.
> 
> I don't understand this. How else can board code work with "comb_iomem"
> than described above? If there is a way, I agree it would be preferable.

object_resolve_path_component(fw_cfg, "fwcfg[0]") and
object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.

I don't know why those names were chosen, though.  Probably it's
a good idea to call object_property_add_child() manually with
more appropriate names inside the fw_cfg code instead of letting
memory_region_init() pick the child name.

-- 
Eduardo