Re: [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

Hi,
It seems your Mail-Followup-To: header causes my client to drop you
from the To: list.

On Wed, Mar 21, 2012 at 11:26 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
>> On Tue, Mar 20, 2012 at 09:54:20AM +0000, Stefan Hajnoczi wrote:
>> > On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
>> > <address@hidden> 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 
>> > >> > interface
>> > >> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
>> > >> > 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 
>> > >> > the
>> > >> > 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 
>> > >> > the
>> > >> > balloon device.
>> > >> >
>> > >> > Cc: Michael S. Tsirkin <address@hidden>
>> > >> > Cc: Rusty Russell <address@hidden>
>> > >> >
>> > >> > Signed-off-by: David Gibson <address@hidden>
>> > >> > ---
>> > >> >  hw/virtio-pci.c |    2 +-
>> > >> >  1 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?  The
>> > >> 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.  But 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.  It was not
>> > > suggetsed by the virtio spec, and it makes no sense.  It 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,
>>
>> Well.. there's the fact that I can't what mechanism we would use to
>> make this per-machine...
>
> Not sure I parsed this correctly, but I think you're asking how to do
> it.
>
> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> .compat_props array that can override qdev properties.
>
> Perhaps Michael Tsirkin or someone else can comment on how to wire up
> hw/virtio-pci.c so that the class code can be overridden.
>
> Stefan