[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 wave 1 2/4] fw-cfg: turn FW_CFG_FILE_SLOTS in

From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH v5 wave 1 2/4] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
Date: Thu, 12 Jan 2017 17:30:42 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0

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),
>>  };
>> @@ -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

> 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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]