Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Mark Cave-Ayland]

On 12/06/17 19:27, Laszlo Ersek wrote:

>> Based upon this what do you think the best solution would be?
> 
> 
> if you apply patch #2 without patch #1, then the above SIGSEGV will hit
> on all fw_cfg using targets / machine types, not just
> qemu-system-sparc64. The reason is that, after patch #2 (without patch
> #1 applied) you have
> 
>   fw_cfg_init1()
>     ...
>       fw_cfg_add_bytes_read_callback()
>         s->entries[arch][key].data
>   qdev_init_nofail()
>     fw_cfg_file_slots_allocate()
> 
> IOW, the s->entries array is now dynamically allocated (after the
> introduction of the x-file-slots property):
> 
>     FWCfgEntry *entries[2];
> 
> and it is allocated in fw_cfg_file_slots_allocate(), called from
> realize. But with patch #2 applied in isolation, you perform the first
> dereference (from fw_cfg_init1(), indirectly) before allocation, that's
> why it crashes.
> 
> Ultimately we need the following order:
> 
> (1) set properties (from device defaults, from device compat settings,
> and from the command line; and also directly from code, such as in
> fw_cfg_init_io_dma())
> 
> (2) call qdev_init_nofail() -- this will consume the properties from
> (1), including the x-file-slots property, for allocating "s->entries" in
> fw_cfg_file_slots_allocate()
> 
> (3) call fw_cfg_add_*() -- now that the device has been realized and is
> usable.
> 
> This means that
> 
> - patch #1 should be dropped,
> 
> - and in patch #2, you have to push the rest of fw_cfg_init1() --
> everything that is after the original qdev_init_nofail() call -- to the
> end of the realize functions.
> 
> 
> Alternatively, in patch #2, you have to split fw_cfg_init1() into two
> parts, fw_cfg_init1() and fw_cfg_init2(). fw_cfg_init1() would keep
> everything that's *before* the original qdev_init_nofail() call, and
> fw_cfg_init2() would get everything *after*. Then, in
> fw_cfg_init_io_dma() and fw_cfg_init_mem_wide(), you'd do
> 
>     fw_cfg_init1(dev);
>     qdev_init_nofail(dev);
>     fw_cfg_init2(dev);
> 
> In short, you cannot reorder steps (2) and (3) against each other -- you
> cannot add fw_cfg entries before you realize the device -- and the crash
> happens because that's exactly what patch #2 does at the moment.
> 
> And, patch #1 is not the right fix (you can allocate s->entries only in
> realize, because the size depends on a property, which you can only
> access in realize). The right fix is to drop patch #1 and to split
> fw_cfg_init1() like described above.
> 
> ... Hmmm, wait a second, while the above approach would fix patch #2, in
> fact I think patch #2 doesn't have the right *goal*.
> 
> I'll comment under patch #2.

Yes that now makes sense - I think I managed to confuse myself when
writing the patch since I grepped for the type and the calls to the
fw_cfg_init_*() functions but managed to miss the x-file-slots property
as pointed out by Igor :(

Reading what you said above, I'd be inclined to move the default values
from fw_cfg_init1() into a QOM method in the parent fw_cfg type as
suggested by Igor, and then call the method right at the end of the
realise function in order to load them.

However it also sounds like you have some better thoughts for patch #2
so I'll wait for your reply to that first.


ATB,

Mark.