qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: Bandan Das <bsd@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-stable <qemu-stable@nongnu.org>,
	qemu-devel@nongnu.org, Anthony Liguori <anthony@codemonkey.ws>,
	Michael Tsirkin <mst@redhat.com>
Subject: Re: [Qemu-devel] [PATCH for-1.7] pci: unregister vmstate_pcibus on unplug
Date: Tue, 19 Nov 2013 18:33:17 +0100	[thread overview]
Message-ID: <528BA0DD.3040902@suse.de> (raw)
In-Reply-To: <jpg7gc4e338.fsf@nelium.bos.redhat.com>

Am 19.11.2013 18:03, schrieb Bandan Das:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 06.11.2013 23:52, schrieb Bandan Das:
>>>
>>> PCIBus registers a vmstate during init. Unregister it upon
>>> removal/unplug.
>>>
>>> Signed-off-by: Bandan Das <bsd@redhat.com>
>>
>> Michael, this patch looks good for 1.7 to me, are you planning to still
>> pick it up? Only one small comment below.
>>
>> Cc: qemu-stable@nongnu.org
>>
>>> ---
>>> Note that I didn't add a instance_init to register vmstate (yet) 
>>> due to concerns expressed by Andreas that we shouldn't be registering 
>>> global state there.
>>
>> What's happening here is the following: instance_init does in fact not
>> register anything, but vmstate_unregister() becomes a no-op loop if the
>> vmsd+opaque combo is not registered, so it is safe. The registration
>> happens in pci_bus_new() / pci_bus_new_inplace(), which I believe all
>> PCI buses to date inside QEMU use, i.e. after instance_init, so in
>> practice unregistering will not be no-op.
> 
> Ok, thanks! Based on your explanation, I think it should be safe to move
> vmstate_register to instance_init as Paolo had suggested.

Why? I still think that would be wrong. We had previously discussed with
Paolo that VMState is global state, which according to Anthony should
not be registered before realization. So far we have a mix of PCI host
bridges instantiating PCIBus before or after realization depending on
whether the bus name needs to depend on the device id or not (with trend
towards instantiating the PCIBus during instance_init), at which point
in time the state should not be registered yet. The sketched solution
was to implement a "realized" property for BusState, so that we can
decouple vmstate_register() from instantation time rather than moving it
into instance_init.

Andreas

> If Michael 
> and rest of the folks agree, I am inclined to send in a new version 
> (which also fixes the issue you noted below).

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

  reply	other threads:[~2013-11-19 17:33 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-06 22:52 [Qemu-devel] [PATCH] pci: unregister vmstate_pcibus on unplug Bandan Das
2013-11-19 12:51 ` [Qemu-devel] [PATCH for-1.7] " Andreas Färber
2013-11-19 13:56   ` Michael S. Tsirkin
2013-11-19 17:03   ` Bandan Das
2013-11-19 17:33     ` Andreas Färber [this message]
2013-11-19 17:44       ` Paolo Bonzini
2013-11-19 17:50       ` Bandan Das

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=528BA0DD.3040902@suse.de \
    --to=afaerber@suse.de \
    --cc=anthony@codemonkey.ws \
    --cc=bsd@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-stable@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).