From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33014) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0xFD-0007aE-7s for qemu-devel@nongnu.org; Tue, 16 Dec 2014 13:53:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0xF6-0000HD-Kg for qemu-devel@nongnu.org; Tue, 16 Dec 2014 13:53:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0xF6-0000Gx-BF for qemu-devel@nongnu.org; Tue, 16 Dec 2014 13:53:08 -0500 Message-ID: <54907F85.8090008@redhat.com> Date: Tue, 16 Dec 2014 19:52:53 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1418399932-7658-1-git-send-email-lersek@redhat.com> <1418399932-7658-4-git-send-email-lersek@redhat.com> <5490203B.2030001@suse.de> <549028B2.9080506@redhat.com> <549064DD.7060707@redhat.com> <549069F7.5030907@suse.de> In-Reply-To: <549069F7.5030907@suse.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Peter Maydell Cc: Andrew Jones , QEMU Developers "Richard W.M. Jones" On 12/16/14 18:20, Alexander Graf wrote: > On 12/16/14 18:10, Peter Maydell wrote: >> On 16 December 2014 at 16:59, Laszlo Ersek wrote: >>> To elaborate on the above -- the fw_cfg device appears to be >>> undestructible at the moment. It has no unrealize callback. If it were >>> destructible, then the above leak would be the smallest of concerns -- >>> it doesn't unmap nor destroy the memory regions that implement the >>> various registers. >>> >>> So, I think the above is not an actual leak, because the result of >>> g_memdup() can never become unreferenced. >> True, and we have a lot of device that are in this same >> category of "can't ever be destroyed". However it is setting >> up a minor beartrap for ourselves in future if we have >> allocations which aren't tracked via a field in the device's >> state structure, because the obvious future implementation of >> destruction for a device is "just free/destroy everything >> that is in the state struct". >> >> NB: I think what Alex had in mind with his option (2) was >> just to have a "MemoryRegionOps ops;" field in the state >> struct, and then use "s->ops = data_mem_ops;" rather than >> the memdup. That retains the use of the static field for >> the non-variable-width case, it's just that instead of >> allocating off the heap for the var-width setup we use >> an inline lump of memory in a struct we're already allocing. >> >> I don't think I care very much about this, but Alex's >> suggestion 2 is slightly nicer I guess. Adding a whole >> unrealize callback is definitely vastly overkill. > > Yeah, it's exactly what I meant. Sorry for not being as clear. By moving > the dynamically created struct into the device struct we're just making > the whole allocation flow easier. > > But if this is the only nitpick, there's no need for a respin just for > that. Okay, I understand it now. Thanks. Laszlo