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-ayl...@ilande.co.uk>
> >> Reviewed-by: Laszlo Ersek <ler...@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
