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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo