[Qemu-devel] Setting FW_CFG_ID (was: [PATCH] fw_cfg: add documentation file)

[Qemu-devel] Setting FW_CFG_ID (was: [PATCH] fw_cfg: add documentation file)

[Gabriel L. Somlo]

On Thu, Mar 19, 2015 at 05:14:04PM +0100, Laszlo Ersek wrote:
> On 03/19/15 01:18, Gabriel L. Somlo wrote:
> 
> > +=== Revision (Key 0x0001, FW_CFG_ID) ===
> > +
> > +A 32-bit little-endian unsigned int, this item is used as an interface
> > +revision number, and is currently set to 1 by all QEMU architectures
> > +which expose a fw_cfg device.
> 
> arm/virt doesn't :)
> 
> It could be argued that that's an error in "hw/arm/virt.c".
> 
> On the other hand, all of the other fw_cfg providing boards set the
> interface version to 1 manually, despite the device coming from the
> same, shared implementation. Therefore one could argue that instead of
> adding
> 
>     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
> 
> to arm/virt, all such existing calls should be consolidated in the
> fw_cfg initialization code instead.
> 
> I guess I must have missed doing it the last time because I had bigger
> fish to fry, and because value 1 for FW_CFG_ID didn't, and doesn't, seem
> to mean anything specific.
> 
> So, I'm just making this note here because I want it on the record that
> I read the above paragraph and didn't miss that it was factually
> incorrect. How it should be fixed, namely:
> - modify the docs,
> - modify the arm/virt board code,
> - move the FW_CFG_ID setting to fw_cfg_init1() -- after all that's
>   where FW_CFG_SIGNATURE is set too --,
> 
> I'll leave up to other reviewers.

I'd be happy to add a patch factoring out the call to

     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);

into fw_cfg_init1(), which would then make it consistent across all
architectures, including arm/virt.

Unless of course anyone chimes in with a good reason for NOT doing
that...

Thanks much,
--Gabriel

Re: [Qemu-devel] Setting FW_CFG_ID (was: [PATCH] fw_cfg: add documentation file)

[Paolo Bonzini]

On 19/03/2015 18:14, Gabriel L. Somlo wrote:
> I'd be happy to add a patch factoring out the call to
> 
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
> 
> into fw_cfg_init1(), which would then make it consistent across all
> architectures, including arm/virt.

Yes, please---this should be a 2.3 blocker.

Paolo