From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54746) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9EYZ-0006CN-7Q for qemu-devel@nongnu.org; Wed, 05 Sep 2012 08:18:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9EYS-0001rt-WD for qemu-devel@nongnu.org; Wed, 05 Sep 2012 08:18:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18744) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9EYS-0001rn-OQ for qemu-devel@nongnu.org; Wed, 05 Sep 2012 08:18:00 -0400 Message-ID: <504742F0.7080907@redhat.com> Date: Wed, 05 Sep 2012 15:17:52 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1345801763-24227-1-git-send-email-qemulist@gmail.com> <503BCCA1.10403@siemens.com> <503C9275.5040105@siemens.com> <503E4FF4.7060506@redhat.com> <503E51BB.9070308@siemens.com> <503E5422.5050805@redhat.com> <503F1172.70103@siemens.com> <5041CB73.4050800@redhat.com> <50446FD9.3040805@redhat.com> <504720F8.9020104@redhat.com> <50472B3B.4090401@siemens.com> <50472F23.2080702@redhat.com> <50473375.8050504@siemens.com> <504736B8.1080706@redhat.com> <50473F3B.4080704@siemens.com> In-Reply-To: <50473F3B.4080704@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Paolo Bonzini , Liu Ping Fan , liu ping fan , Anthony Liguori , "qemu-devel@nongnu.org" On 09/05/2012 03:02 PM, Jan Kiszka wrote: > On 2012-09-05 13:25, Avi Kivity wrote: >> On 09/05/2012 02:11 PM, Jan Kiszka wrote: >>> On 2012-09-05 12:53, Avi Kivity wrote: >>>> On 09/05/2012 01:36 PM, Jan Kiszka wrote: >>>>>> >>>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a >>>>>> corresponding unref), which has the following requirements: >>>>>> >>>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use >>>>>> - if the refcount is nonzero, the MemoryRegion itself is stable. >>>>> >>>>> The second point means that the memory subsystem will cache the region >>>>> state and apply changes only after leaving a handler that performed them? >>>> >>>> No. I/O callbacks may be called after a region has been disabled. >>>> >>>> I guess we can restrict this to converted regions, so we don't introduce >>>> regressions. >>> >>> s/can/have to/. This property change will require some special care, >>> hopefully mostly at the memory layer. A simple scenario from recent patches: >>> >>> if () { >>> memory_region_set_address(&s->pm_io, pm_io_base); >>> memory_region_set_enabled(&s->pm_io, true); >>> } else { >>> memory_region_set_enabled(&s->pm_io, false); >>> } >> >> I am unable to avoid pointing out that this can be collapsed to >> >> memory_region_set_address(&s->pm_io, pm_io_base); >> memory_region_set_enabled(&s->pm_io, ); >> >> as the address is meaningless when disabled. Sorry. >> >>> >>> We will have to ensure that set_enabled(..., true) will never cause a >>> dispatch using an outdated base address. >> >> No, this is entirely safe. If the guest changes a region offset >> concurrently with issuing mmio on it, then it must expect either the old >> or new offset to be used during dispatch. > I forgot to mention that my clever rewrite above actually breaks this - it needs to be wrapped in a transaction, otherwise we have a move-then-disable pattern. > You assume disable, reprogram, enable, ignoring that there could be > multiple, invalid states between disable and enable. Real hardware takes > care of the ordering. It's possible of course, but the snippet above isn't susceptible on its own. I don't think it's solvable. To serialize, you must hold the device lock, but we don't want to take the device lock during dispatch. Users can protect against them by checking for ->enabled: void foo_io_read(...) { FooState *s = container_of(mr, ...); qemu_mutex_lock(&s->lock); if (!memory_region_enabled(mr)) { *data = ~(uint64_t)0; goto out; } ... out: qemu_mutex_unlock(&s->lock); } Non-converted users will be naturally protected since we will take the bql on their behalf. -- error compiling committee.c: too many arguments to function