From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46757) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cRiH5-0002d4-US for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:30:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cRiH0-0003Q1-VN for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:30:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58116) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cRiH0-0003PP-MV for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:30:46 -0500 References: <20170111173457.30455-1-lersek@redhat.com> <20170111173457.30455-3-lersek@redhat.com> <20170112131053.GA27893@thinpad.lan.raisama.net> From: Laszlo Ersek Message-ID: <899dc23a-b6c0-e9c8-c31a-03e4cc8e7b7e@redhat.com> Date: Thu, 12 Jan 2017 17:30:42 +0100 MIME-Version: 1.0 In-Reply-To: <20170112131053.GA27893@thinpad.lan.raisama.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 wave 1 2/4] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu devel list , Igor Mammedov , "Gabriel L. Somlo" , Paolo Bonzini , Gerd Hoffmann , "Michael S. Tsirkin" 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