From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9vlt-0005br-Lb for qemu-devel@nongnu.org; Tue, 20 Mar 2012 05:54:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9vlo-0005Kg-Cf for qemu-devel@nongnu.org; Tue, 20 Mar 2012 05:54:29 -0400 MIME-Version: 1.0 In-Reply-To: <20120320004206.GB22089@truffala.fritz.box> References: <1332133163-7890-1-git-send-email-david@gibson.dropbear.id.au> <20120319113310.GD30033@stefanha-thinkpad.localdomain> <20120320004206.GB22089@truffala.fritz.box> Date: Tue, 20 Mar 2012 09:54:20 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: anthony@codemonkey.ws Cc: "Michael S. Tsirkin" , qemu-trivial@nongnu.org, Stefan Hajnoczi , Rusty Russell , qemu-devel@nongnu.org, David Gibson On Tue, Mar 20, 2012 at 12:42 AM, David Gibson wrote: > On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote: >> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote: >> > Currently the virtio balloon device, when using the virtio-pci interfa= ce >> > advertises itself with PCI class code MEMORY_RAM. =A0This is wrong; th= e >> > balloon is vaguely related to memory, but is nothing like a PCI memory >> > device in the meaning of the class code, and this code is not required= or >> > suggested by the virtio PCI specification. >> > >> > Worse, this patch causes problems on the pseries machine, because the >> > firmware, seeing this class code, advertises the device as memory in t= he >> > device tree, and then a guest kernel bug causes it to see this "memory= " >> > before the real system memory, leading to a crash in early boot. >> > >> > This patch fixes the problem by removing the bogus PCI class code on t= he >> > balloon device. >> > >> > Cc: Michael S. Tsirkin >> > Cc: Rusty Russell >> > >> > Signed-off-by: David Gibson >> > --- >> > =A0hw/virtio-pci.c | =A0 =A02 +- >> > =A01 files changed, 1 insertions(+), 1 deletions(-) >> >> Since this is a guest-visible change we might need to be careful about >> how it's introduced. >> >> Do we need to keep the old class code for existing machine types? =A0The >> new class code could be introduced only for 1.1 and later machine types >> if we want to be extra careful about introducing guest-visible >> changes. > > So as a general rule, I like to be very careful about user-visible > changes. =A0But in this case, I don't think we want to be too hesitant. > In particular, it's not just a question of the machine type, but also > of how the guest OS will deal with the PCI class code. > > The class code we were using was Just Plain Wrong. =A0It was not > suggetsed by the virtio spec, and it makes no sense. =A0It happens that > so far this caused problems only for a guest on a particular machine > type, but there's no reason it couldn't cause (different) problems for > guests on any machine type. > > More to the point, it seems reasonably unlikely for existing guests to > rely on the broken behaviour: again, there's no reason they'd think > they need to based on the spec, and the usual way of matching drivers > to PCI devices is with the vendor/device IDs which are correct and not > changed by this patch. > > So, unless we have a known example of an existing guest that would be > broken by this change, I think we should implement it ASAP for all > machine types. I agree that in practice the risk is low because working guests are probably not using the class code. On the other hand I don't see a downside to making this part of the 1.1 machine type, which is what users will run when they get this code change anyway. That way we can tell users that we never change the device model in a release with a straight face :). Anthony: I'm not sure how strict we are about a user-visible change like th= is? Stefan