qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" pr


From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property
Date: Tue, 16 Dec 2014 18:20:55 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0

On 12/16/14 18:10, Peter Maydell wrote:
On 16 December 2014 at 16:59, Laszlo Ersek <address@hidden> 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.


Alex




reply via email to

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