This brings the function in line with fw_cfg_mem_realize(), deferring the
actual mapping until outside of the realize function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 87b4392..4159316 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
     FWCfgState *s;
     bool dma_requested = dma_iobase && dma_as;
 
@@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
 
     qdev_init_nofail(dev);
 
+    sbd = SYS_BUS_DEVICE(dev);
+    sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
+        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
     }
 
     return s;
@@ -1090,13 +1095,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
-    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
+    sysbus_init_mmio(sbd, &s->comb_iomem);
 
     if (FW_CFG(s)->dma_enabled) {
         memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
-        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
+        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
 }
 
-- 
1.7.10.4

On Sat, 10 Jun 2017 13:30:20 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> This brings the function in line with fw_cfg_mem_realize(), deferring the
> actual mapping until outside of the realize function.
you are changing used API here, so add to commit message why is that

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 87b4392..4159316 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
>      FWCfgState *s;
>      bool dma_requested = dma_iobase && dma_as;
>  
> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>  
>      qdev_init_nofail(dev);
>  
> +    sbd = SYS_BUS_DEVICE(dev);
> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
so I'd use that if APIs are switched.

> +
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
>          /* 64 bits for the address field */
>          s->dma_as = dma_as;
>          s->dma_addr = 0;
> +        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
>      }
>  
>      return s;
> @@ -1090,13 +1095,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>       * of the i/o region used is FW_CFG_CTL_SIZE */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>                            FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
> -    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
> +    sysbus_init_mmio(sbd, &s->comb_iomem);
>  
>      if (FW_CFG(s)->dma_enabled) {
>          memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
> -        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
>  }
>  

On 12/06/17 13:16, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:20 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> This brings the function in line with fw_cfg_mem_realize(), deferring the
>> actual mapping until outside of the realize function.
> you are changing used API here, so add to commit message why is that

Okay I can add a comment mentioning that this is so we can wire up the
IO space ourselves?

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 87b4392..4159316 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>                                  AddressSpace *dma_as)
>>  {
>>      DeviceState *dev;
>> +    SysBusDevice *sbd;
>>      FWCfgState *s;
>>      bool dma_requested = dma_iobase && dma_as;
>>  
>> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>  
>>      qdev_init_nofail(dev);
>>  
>> +    sbd = SYS_BUS_DEVICE(dev);
>> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
> sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
> so I'd use that if APIs are switched.

Unfortunately we can't use sysbus_mmio_map() here because it maps the
resulting MemoryRegion into memory space instead of IO space.

The reason for using sysbus_init_mmio() here is that despite its name,
we can use sysbus_mmio_get_region() to obtain a reference to the
underlying s->comb_iomem MemoryRegion that the caller can use in order
to map the device into the desired IO space,

Otherwise if we use sysbus_add_io() at realize time as per the existing
code then the ioports are always mapped into system IO space which is
exactly the behaviour I'm trying to customise.

Note that this is how the m48t59 timer device is currently implemented
in hw/timer/m48t59.c.

> 
>> +
>>      s = FW_CFG(dev);
>>  
>>      if (s->dma_enabled) {
>>          /* 64 bits for the address field */
>>          s->dma_as = dma_as;
>>          s->dma_addr = 0;
>> +        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
>>      }
>>  
>>      return s;
>> @@ -1090,13 +1095,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>       * of the i/o region used is FW_CFG_CTL_SIZE */
>>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>>                            FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
>> -    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
>> +    sysbus_init_mmio(sbd, &s->comb_iomem);
>>  
>>      if (FW_CFG(s)->dma_enabled) {
>>          memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
>>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>>                                sizeof(dma_addr_t));
>> -        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
>> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>>      }
>>  }
>>  


ATB,

Mark.