diff mbox

[2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

Message ID 1497097821-32754-3-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Cave-Ayland June 10, 2017, 12:30 p.m. UTC 
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Igor Mammedov June 12, 2017, 11:43 a.m. UTC | #1 
On Sat, 10 Jun 2017 13:30:17 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

patch needs a commit message saying why it's needed.
maybe add something similar to:

qdev_init_nofail() essentially 'realizes' object,
taking in account that fw_cfg_init1() will be moved to
realize in follow up patches, move qdev_init_nofail() outside of
fw_cfg_init1().



> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 144e0c6..1313bfd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -919,8 +919,6 @@ static void fw_cfg_init1(DeviceState *dev)
>  
>      object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>  
> -    qdev_init_nofail(dev);
> -
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> @@ -948,6 +946,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      }
>  
>      fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
> +
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
> @@ -985,6 +985,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>      }
>  
>      fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);
Mark Cave-Ayland June 12, 2017, 11:54 a.m. UTC | #2 
On 12/06/17 12:43, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:17 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> patch needs a commit message saying why it's needed.
> maybe add something similar to:
> 
> qdev_init_nofail() essentially 'realizes' object,
> taking in account that fw_cfg_init1() will be moved to
> realize in follow up patches, move qdev_init_nofail() outside of
> fw_cfg_init1().

Yes, I can alter the commit message to provide more explanation. For
some background the use case can be found in my follow-up sun4u patches
here (i.e. preparation for moving the ebus device behind a PCI bridge):

https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html


ATB,

Mark.
Laszlo Ersek June 12, 2017, 7:15 p.m. UTC | #3 
Adding Peter

On 06/12/17 13:54, Mark Cave-Ayland wrote:
> On 12/06/17 12:43, Igor Mammedov wrote:
> 
>> On Sat, 10 Jun 2017 13:30:17 +0100
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> patch needs a commit message saying why it's needed.
>> maybe add something similar to:
>>
>> qdev_init_nofail() essentially 'realizes' object,
>> taking in account that fw_cfg_init1() will be moved to
>> realize in follow up patches, move qdev_init_nofail() outside of
>> fw_cfg_init1().
> 
> Yes, I can alter the commit message to provide more explanation. For
> some background the use case can be found in my follow-up sun4u patches
> here (i.e. preparation for moving the ebus device behind a PCI bridge):
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html

I see.

So what you want to replace actually resides in fw_cfg_io_realize(). It
is the following set of function calls:

    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);

        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);

which both translate to

    memory_region_add_subregion(get_system_io(), addr, mem);

And you want to replace the first parameter here, namely get_system_io().

I think your use case uncovered an actual QOM bug in
fw_cfg_io_realize(). Compare fw_cfg_mem_realize():

    sysbus_init_mmio(sbd, &s->ctl_iomem);

    sysbus_init_mmio(sbd, &s->data_iomem);

        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);

But these function calls do not *map* subregions!

Now, I *distinctly* recall that, when we were working on
fw_cfg_mem_realize(), Paolo and Peter pointed out that, while MMIO and
IO region *creation* were "realize business", *mapping* those same
regions to address spaces (or higher level memory regions) was *board*
business... Yes, please see the following messages:

http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02839.html
http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02849.html

Ultimately, Paolo graciously fixed up this error in my then-v5 patch
(thanks again Paolo!); see:

- http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02851.html
- http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03254.html
- commit 5712db6ae510 ("fw_cfg: hard separation between the MMIO and
  I/O port mappings", 2014-12-22).

But, we all seem to have missed the exact same error in
fw_cfg_io_realize() at that time.

So here's my suggestion:

(1) Fix the QOM bug -- my mess :) -- at the very first. Move the
sysbus_add_io() calls from fw_cfg_io_realize() to fw_cfg_init_io_dma()
(which is a board helper function, not a realize function), after
fw_cfg_init1().

Notice that in fw_cfg_init_mem_wide(), we have

    fw_cfg_init1(dev);

    sbd = SYS_BUS_DEVICE(dev);
    sysbus_mmio_map(sbd, 0, ctl_addr);
    sysbus_mmio_map(sbd, 1, data_addr);

which is exactly what we should parallel in fw_cfg_init_io_dma().

(2) Once this is done, modify the fw_cfg_init_io_dma() function, so that
it takes a new MemoryRegion* parameter called "parent_io". Update all
current call sites to pass in NULL, and fw_cfg_init_io_dma() should do
something like:

  io = parent_io ? parent_io : get_system_io();
  memory_region_add_subregion(io, s->iobase, &s->comb_iomem);
  if (s->dma_enabled) {
    memory_region_add_subregion(io, s->dma_iobase, s->dma_iomem);
  }


Basically, fix the QOM bug first, by moving the region mapping out of
the realize function, to the board helper function. Then modify the
board helper function so that board code can pass in the parent_io region.

This should be two small patches, and the rest should be possible to
drop, I think.

Then, in your sun4u series, you can simply remove

    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);

and add

    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, 0, NULL,
                                pci_address_space_io(ebus));

... Upon re-reading your cover letter:

> As part of some ongoing sun4u work, I need to be able to wire the
> fw_cfg IO interface to a separate IO space by instantiating the qdev
> device instead of calling fw_cfg_init_io(). This patchset brings
> FW_CFG_IO in line with FW_CFG_MEM and tidies up the realize methods
> accordingly.

I agree that fw_cfg_init_io() should be synched with fw_cfg_init_mem(),
in that the region *mapping* should be moved from the realize function
to the board helper function.

Beyond that, I don't think that a whole lot of code movement /
reorganization is necessary. Simply empower the current board helper
function fw_cfg_init_io_dma() to take parent_io, and then use that from
sun4u board code.

Thanks
Laszlo
Mark Cave-Ayland June 12, 2017, 7:50 p.m. UTC | #4 
On 12/06/17 20:15, Laszlo Ersek wrote:

> Adding Peter
> 
> On 06/12/17 13:54, Mark Cave-Ayland wrote:
>> On 12/06/17 12:43, Igor Mammedov wrote:
>>
>>> On Sat, 10 Jun 2017 13:30:17 +0100
>>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> patch needs a commit message saying why it's needed.
>>> maybe add something similar to:
>>>
>>> qdev_init_nofail() essentially 'realizes' object,
>>> taking in account that fw_cfg_init1() will be moved to
>>> realize in follow up patches, move qdev_init_nofail() outside of
>>> fw_cfg_init1().
>>
>> Yes, I can alter the commit message to provide more explanation. For
>> some background the use case can be found in my follow-up sun4u patches
>> here (i.e. preparation for moving the ebus device behind a PCI bridge):
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html
> 
> I see.
> 
> So what you want to replace actually resides in fw_cfg_io_realize(). It
> is the following set of function calls:
> 
>     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
> 
>         sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> 
> which both translate to
> 
>     memory_region_add_subregion(get_system_io(), addr, mem);
> 
> And you want to replace the first parameter here, namely get_system_io().
> 
> I think your use case uncovered an actual QOM bug in
> fw_cfg_io_realize(). Compare fw_cfg_mem_realize():
> 
>     sysbus_init_mmio(sbd, &s->ctl_iomem);
> 
>     sysbus_init_mmio(sbd, &s->data_iomem);
> 
>         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
> 
> But these function calls do not *map* subregions!
> 
> Now, I *distinctly* recall that, when we were working on
> fw_cfg_mem_realize(), Paolo and Peter pointed out that, while MMIO and
> IO region *creation* were "realize business", *mapping* those same
> regions to address spaces (or higher level memory regions) was *board*
> business... Yes, please see the following messages:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02839.html
> http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02849.html
> 
> Ultimately, Paolo graciously fixed up this error in my then-v5 patch
> (thanks again Paolo!); see:
> 
> - http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02851.html
> - http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03254.html
> - commit 5712db6ae510 ("fw_cfg: hard separation between the MMIO and
>   I/O port mappings", 2014-12-22).
> 
> But, we all seem to have missed the exact same error in
> fw_cfg_io_realize() at that time.
> 
> So here's my suggestion:
> 
> (1) Fix the QOM bug -- my mess :) -- at the very first. Move the
> sysbus_add_io() calls from fw_cfg_io_realize() to fw_cfg_init_io_dma()
> (which is a board helper function, not a realize function), after
> fw_cfg_init1().
> 
> Notice that in fw_cfg_init_mem_wide(), we have
> 
>     fw_cfg_init1(dev);
> 
>     sbd = SYS_BUS_DEVICE(dev);
>     sysbus_mmio_map(sbd, 0, ctl_addr);
>     sysbus_mmio_map(sbd, 1, data_addr);
> 
> which is exactly what we should parallel in fw_cfg_init_io_dma().
> 
> (2) Once this is done, modify the fw_cfg_init_io_dma() function, so that
> it takes a new MemoryRegion* parameter called "parent_io". Update all
> current call sites to pass in NULL, and fw_cfg_init_io_dma() should do
> something like:
> 
>   io = parent_io ? parent_io : get_system_io();
>   memory_region_add_subregion(io, s->iobase, &s->comb_iomem);
>   if (s->dma_enabled) {
>     memory_region_add_subregion(io, s->dma_iobase, s->dma_iomem);
>   }
> 
> 
> Basically, fix the QOM bug first, by moving the region mapping out of
> the realize function, to the board helper function. Then modify the
> board helper function so that board code can pass in the parent_io region.
> 
> This should be two small patches, and the rest should be possible to
> drop, I think.
> 
> Then, in your sun4u series, you can simply remove
> 
>     fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> 
> and add
> 
>     fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, 0, NULL,
>                                 pci_address_space_io(ebus));
> 
> ... Upon re-reading your cover letter:
> 
>> As part of some ongoing sun4u work, I need to be able to wire the
>> fw_cfg IO interface to a separate IO space by instantiating the qdev
>> device instead of calling fw_cfg_init_io(). This patchset brings
>> FW_CFG_IO in line with FW_CFG_MEM and tidies up the realize methods
>> accordingly.
> 
> I agree that fw_cfg_init_io() should be synched with fw_cfg_init_mem(),
> in that the region *mapping* should be moved from the realize function
> to the board helper function.
> 
> Beyond that, I don't think that a whole lot of code movement /
> reorganization is necessary. Simply empower the current board helper
> function fw_cfg_init_io_dma() to take parent_io, and then use that from
> sun4u board code.

Thanks a lot for the clarification! I agree that (1) is definitely a
bug, however I'm not keen on (2) since by adding the parent MemoryRegion
as a parameter to fw_cfg_init_io_dma() then we've lost the symmetry
between that and fw_cfg_init_mem_wide(), and I do feel that realize
should be putting in the shared defaults itself.

Also the general "feel" of these APIs is that the _init() function sets
the defaults while instantiating the device with qdev_init_nofail()
allows you to wire up the device as required.

Now I understand this much better, let me try a v2 of this which fixes
(1) and moves fw_cfg_init1() to a QOM method in the parent as suggested
by Igor and see what you think.


ATB,

Mark.
Mark Cave-Ayland June 12, 2017, 9:11 p.m. UTC | #5 
On 12/06/17 20:50, Mark Cave-Ayland wrote:

> Now I understand this much better, let me try a v2 of this which fixes
> (1) and moves fw_cfg_init1() to a QOM method in the parent as suggested
> by Igor and see what you think.

Experimenting with this some more, if the aim is to try and minimise
code movement/reorganisation then I don't think the QOM method will give
that much benefit for the work involved.

I've just created a v2 with the aim of minimising code churn which I'll
send along shortly.


ATB,

Mark.
diff mbox

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 144e0c6..1313bfd 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -919,8 +919,6 @@  static void fw_cfg_init1(DeviceState *dev)
 
     object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
 
-    qdev_init_nofail(dev);
-
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
@@ -948,6 +946,8 @@  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     }
 
     fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
@@ -985,6 +985,7 @@  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     }
 
     fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);