From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35666) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dkBKg-0004Cp-KC for qemu-devel@nongnu.org; Tue, 22 Aug 2017 11:43:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dkBKf-0003Mn-Ou for qemu-devel@nongnu.org; Tue, 22 Aug 2017 11:43:10 -0400 References: <1503414375-21009-1-git-send-email-thuth@redhat.com> <443135b4-4b36-bae6-b50a-12fb4a55a858@redhat.com> From: Thomas Huth Message-ID: <06c16109-cdd2-0732-176d-6ec80b1f7f4e@redhat.com> Date: Tue, 22 Aug 2017 17:42:59 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/misc/auxbus.c: Mark the aux-to-i2c-bridge device as non-hotpluggable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-arm , QEMU Developers , "Edgar E. Iglesias" , Alistair Francis , KONRAD Frederic , QEMU Trivial , Markus Armbruster , Eduardo Habkost On 22.08.2017 17:30, Peter Maydell wrote: > On 22 August 2017 at 16:26, Thomas Huth wrote: >> On 22.08.2017 17:12, Peter Maydell wrote: >>> Why is our default "hotpluggable" rather than "not hotpluggable" ? >>> We must have way more non-hotpluggable devices than hotpluggable >>> ones, and it takes active effort to make a hotpluggable device >>> model, so it seems like it would be much less bug-prone to >>> require hotpluggable devices to set dc->hotpluggable true >>> rather than all the non-hotpluggable ones to set it false... >> >> I think most devices are already non-hotpluggable automatically because >> they sit on a bus that is not hot-pluggable (e.g. sysbus devices). The >> problematic ones are the devices with .parent = TYPE_DEVICE. And for >> these, I think it is quite hard to say whether they should be >> hot-pluggable by default or not? > > To implement hotplug you need to write extra code (notably some > kind of unrealize method to undo whatever you did in realize), > at which point also setting the hotpluggable flag is trivial. > >> Anyway, according to my tests (I'm currently working on a test that >> automatically does device_add + device_del for all devices, as you might >> have guessed already), there are not that many devices that cause >> problems here, so I guess marking some few with hotpluggable = false >> should be OK? > > The problem is not the devices we have today but the ones we're > going to write tomorrow. Having hotpluggable default to false > "fails safe" -- the worst that happens is that somebody writing > a hotpluggable device finds in their testing that they need to > set the flag to make it work. Having it default to true fails > non-safely -- as you've found, people writing devices that > were never expected to be hotplugged don't try to test the > error case of attempting hotplug anyway, and in code review > the absence of a line of code is very hard to reliably spot, > so we get devices in tree that crash QEMU if you try to > hotplug them. If we make the default be not-hotpluggable > we fix this not just today but forever. OK, you've got a point. I'll ponder about this a little bit and will try to come up with a patch / some patches ... Thomas