Re: [Qemu-devel] [PATCH v5 wave 1 2/4] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property

[Laszlo Ersek]

On 01/12/17 14:10, Eduardo Habkost wrote:
> On Wed, Jan 11, 2017 at 06:34:55PM +0100, Laszlo Ersek wrote:
> [...]
>>  static Property fw_cfg_io_properties[] = {
>>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>>                       true),
>> +    DEFINE_PROP_UINT16("file_slots", FWCfgIoState, parent_obj.file_slots,
>> +                       FW_CFG_FILE_SLOTS_MIN),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> @@ -1027,6 +1066,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error 
>> **errp)
> 
> I'm not sure what is more important here: following the QOM
> property name convention using "-" instead of "_", or being
> consistent with the other existing properties.

Right, when working with the compat settings, I saw both schemes. I
couldn't decide. I figured I'd stick with the underscore seen with
"dma_enabled".

> 
> In either case, we could add a "x-" prefix to indicate it is not
> supposed to be configured directly by the user.

I thought "x-" meant "experimental"; i.e., the property could be changed
or could go away at any moment. That's a slightly different meaning than
"not meant for users".

BTW I think the same (== not meant for the user) applies to a number of
other properties (dma_enabled is one of them, but other devices have
this kind too); do we consistently call them x-*?

> 
> [...]
>> @@ -1063,6 +1109,8 @@ static Property fw_cfg_mem_properties[] = {
>>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>>                       true),
>> +    DEFINE_PROP_UINT16("file_slots", FWCfgMemState, parent_obj.file_slots,
>> +                       FW_CFG_FILE_SLOTS_MIN),
> 
> It looks like you can add the property to the TYPE_FW_CFG parent
> class instead of duplicating it on the subclasses. The existing
> "dma_enabled" property could be moved there as well.

I would prefer if someone could pick up that task, separately.

The idea crossed my mind when I was working on the common base class,
originally, wrt. dma_enabled. I vaguely remember that there was some
problem with moving up the property. Ultimately the reviewers didn't
expect me, or suggest, to move it up, hence the current status.

I think it's out of scope for this series. If you (or someone else) can
do it, I agree it would be an improvement, but I'd rather learn the
method from someone else's patch than experiment with it myself.


If there's a consensus on the x- prefix and/or the underscore/hyphen
question, I'm happy to send a v6 with those changes.

Thanks,
Laszlo