Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Mark Cave-Ayland]

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 <address@hidden> 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.