From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50782) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cvOAH-0007wW-4v for qemu-devel@nongnu.org; Tue, 04 Apr 2017 09:06:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cvOAF-00074i-RQ for qemu-devel@nongnu.org; Tue, 04 Apr 2017 09:06:29 -0400 Date: Tue, 4 Apr 2017 10:05:50 -0300 From: Eduardo Habkost Message-ID: <20170404130550.GR1910@thinpad.lan.raisama.net> References: <20170401004624.30886-1-ehabkost@redhat.com> <20170401004624.30886-4-ehabkost@redhat.com> <20170403201026.GJ1910@thinpad.lan.raisama.net> <20170403210030.GK1910@thinpad.lan.raisama.net> <522a3489-2e8c-b31a-c592-391fe5cf3b5f@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <522a3489-2e8c-b31a-c592-391fe5cf3b5f@suse.de> Subject: Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Peter Maydell , QEMU Developers , Laszlo Ersek , Marcel Apfelbaum , Thomas Huth , Markus Armbruster , John Snow , Kevin Wolf , Max Reitz , Paolo Bonzini , Richard Henderson , "Michael S. Tsirkin" , Scott Wood , Jason Wang , David Gibson , Gerd Hoffmann , Alex Williamson , Qemu-block , "qemu-ppc@nongnu.org" , Alistair Francis , Beniamino Galvani , "Edgar E. Iglesias" , "Gabriel L . Somlo" , Igor Mammedov , Prasad J Pandit , qemu-arm , Shannon Zhao On Tue, Apr 04, 2017 at 08:53:43AM +0200, Alexander Graf wrote: > > > On 03.04.17 23:00, Eduardo Habkost wrote: > > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote: > > > > > > > > > On 03.04.17 22:10, Eduardo Habkost wrote: > > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote: > > > > > On 1 April 2017 at 01:46, Eduardo Habkost wrote: > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making > > > > > > all kinds of untested devices available to -device and > > > > > > device_add. > > > > > > > > > > > > The problem with that is: setting has_dynamic_sysbus on a > > > > > > machine-type lets it accept all the 288 sysbus device types we > > > > > > have in QEMU, and most of them were never meant to be used with > > > > > > -device. That's a lot of untested code. > > > > > > > > > > > > Fortunately today we have just a few has_dynamic_sysbus=1 > > > > > > machines: virt, pc-q35-*, ppce500, and spapr. > > > > > > > > > > > > virt, ppce500, and spapr have extra checks to ensure just a few > > > > > > device types can be instantiated: > > > > > > > > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE. > > > > > > * ppce500 supports only TYPE_ETSEC_COMMON. > > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE. > > > > > > > > > > > > q35 has no code to block unsupported sysbus devices, however, and > > > > > > accepts all device types. Fortunately, only the following 20 > > > > > > device types are compiled into the qemu-system-x86_64 and > > > > > > qemu-system-i386 binaries: > > > > > > > > > > > > * allwinner-ahci > > > > > > * amd-iommu > > > > > > * cfi.pflash01 > > > > > > * esp > > > > > > * fw_cfg_io > > > > > > * fw_cfg_mem > > > > > > * generic-sdhci > > > > > > * hpet > > > > > > * intel-iommu > > > > > > * ioapic > > > > > > * isabus-bridge > > > > > > * kvmclock > > > > > > * kvm-ioapic > > > > > > * kvmvapic > > > > > > * SUNW,fdtwo > > > > > > * sysbus-ahci > > > > > > * sysbus-fdc > > > > > > * sysbus-ohci > > > > > > * unimplemented-device > > > > > > * virtio-mmio > > > > > > > > > > > > Instead of requiring each machine-type with has_dynamic_sysbus=1 > > > > > > to implement its own mechanism to block unsupported devices, we > > > > > > can use the user_creatable flag to ensure we won't let the user > > > > > > plug anything that will never work. > > > > > > > > > > How does this work? Which devices can be dynamically > > > > > plugged is machine dependent. You can't dynamically-plug > > > > > an intel-iommu on the ARM virt board, and you can't > > > > > dynamically-plug the vfio-calxeda-xgmac on the spapr > > > > > board, and so on. So I don't see how we can just have > > > > > a flag on the device itself that controls whether > > > > > it can be dynamically plugged. > > > > > > > > > > So I'm definitely coming around to the opinion that > > > > > it's just a bug in the q35 board that it doesn't have > > > > > any device whitelisting, and we should fix that. > > > > > > > > OK, let's assume q35 must implement a whitelist: > > > > > > > > To build that whitelist, we need to be able to know what should > > > > be in the whitelist, or not. And nobody knew for sure what was > > > > user-creatable in q35 by accident, and what was really supposed > > > > to be user-creatable in q35. See the "q35 and sysbus devices" > > > > thread I started ~2 weeks ago. > > > > > > > > Building a q35 whitelist will be much easier if make > > > > sys-bus-devices non-user-creatable by default. > > > > > > So why are they user creatable in the first place? > > > > > > We used to have boards that were dynamic sysbus aware (ppce500, virt) that > > > allowed dynamic creation and every other board did not. I don't remember the > > > exact mechanism behind it though. > > > > > > When did that behavior change? It sounds like a regression somewhere. > > > > See patch description: > > > > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset > > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, > > > > Note that the commit above is not a regression[1] (because q35 > > didn't have has_dynamic_sysbus=1 yet), but having sysbus devices > > have cannot_instantiate_with_device_add_yet=false/user_creatable=true > > by default makes it harder to build the whitelist for q35 (or > > other machines that will have has_dynamic_sysbus=1 in the > > future). > > I seem to miss the bigger picture. > > Why would anyone set has_dynamic_sysbus=1 in a board file without an > explicit whitelist? I guess it was because this was not documented anywhere. Note that q35 is not the only case today: see xen_set_dynamic_sysbus(). > The whitelist is *not* device specific. It's board > specific, because your board needs to know how to wire up a device and how > to expose the fact that it exists to the OS. That part is true. > > So the real bug is that someone set has_dynamic_sysbus=1 in q35 without > implementing all of the dynamic sysbus logic, no? This part I disagree: we don't have "a real bug", we have two different bugs that we are mixing them up: 1) q35 and the xen_set_dynamic_sysbus() hack have no whitelist. 2) cannot_instantiate_with_device_add_yet is not set correctly on sysbus device classes (breaking "info qdm", and maybe other code that depends on cannot_instantiate_with_device_add_yet). It's my fault that we're mixing them up, because I am using bug #1 as justification for the #2 fix (this seris). But fixing #2 is not necessary for fixing #1, it just helps a lot. -- Eduardo