On Tue, May 07, 2013 at 05:29:34PM +0200, KONRAD Frédéric wrote:
> On 07/05/2013 16:00, Michael S. Tsirkin wrote:
> >On Tue, May 07, 2013 at 02:54:38PM +0200, KONRAD Frédéric wrote:
> >>On 07/05/2013 14:33, Michael S. Tsirkin wrote:
> >>>On Tue, May 07, 2013 at 02:29:38PM +0200, KONRAD Frédéric wrote:
> >>>>Hi,
> >>>>
> >>>>I think it is not a good idea, as we wanted to make VirtIODevice
> >>>>hot-pluggable for virtio-mmio.
> >>>And then this hack will break cross version migration.
> >>Why?
> >>
> >>virtio_net_set_config_size is called by each "proxy" devices no?
> >If it's called then there's no need for a default,
> >so this patch should be fine.
>
> True but as I told you, we made this refactoring to be able to hot-plug
> VirtIODevice on a virtio-bus, so calling this function won't be possible.
Going in circles aren't we? If it's not called
it's a bug. If it's called default is not needed.
> But you are right this must be fixed.
>
> >
> >>>>Maybe we can use a callback which is called by virtio-bus before
> >>>>plugging, to ensure that this
> >>>>config size is computed?
> >>>OK, will you post such a patch?
> >>>
> >>The issue is as we said in the last thread, host_feature is part of
> >>the proxy.
> >Maybe you could add documentation on how initialization works?
> >If I could fiure it out I would maybe suggest some solutions.
>
> Yes, where do you think I can add this?
Everywhere. Seriously.
Start with files. File headers should give some hint
as to what's in here
* VirtioBus
in virtio-bus.h and virtio-bus.c is not helpful.
How about
VirtioBus - a bus that drives from Newcastle to Edinburgh
each Tuesday at noon. Drives back on Wednesdays mostly empty.
It serves as a parent class for all the small taxi cars
in both of these towns.
or some such.
Go over the interfaces and document what they do.
I mean more than just repeat the name:
/* Destroy the VirtIODevice */
void virtio_bus_destroy_device(VirtioBusState *bus)
is not really helpful.
/* Destroy a device. Assumes you
don't have valuables in there. Calls 911 automatically. */
same for
/* Plug the VirtIODevice */
int virtio_bus_plug_device(VirtIODevice *vdev)
did you mean
/* Plug the device so the content does not
spill. This way you can take put it on the bus.
You must use a corkscrew to unplug it later.
*/
> >>And if we want to move it to the devices, we must have a kind of
> >>property forwarding mechanism.
> >>
> >>Anthony said he had something about that.
> >>>>>All buses do this, and if they don't they get subtle
> >>>>>bugs related to cross version migration.
> >>>>>Let's add an assert to catch these bugs early.
> >>>>>
> >>>>>Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> >>>>>---
> >>>>>
> >>>>>Just a cleanup so not 1.5 material.
> >>>>>Seems to work fine here - any opinions?
> >>>>>
> >>>>> hw/net/virtio-net.c | 7 ++++---
> >>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>
> >>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>index 908e7b8..f0a9272 100644
> >>>>>--- a/hw/net/virtio-net.c
> >>>>>+++ b/hw/net/virtio-net.c
> >>>>>@@ -1282,6 +1282,8 @@ static int virtio_net_device_init(VirtIODevice
> >>>>>*vdev)
> >>>>> DeviceState *qdev = DEVICE(vdev);
> >>>>> VirtIONet *n = VIRTIO_NET(vdev);
> >>>>>+ /* Verify that config size has been initialized */
> >>>>>+ assert(n->config_size != (size_t)-1);
> >>>>> virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> >>>>> n->config_size);
> >>>>>@@ -1386,10 +1388,9 @@ static void virtio_net_instance_init(Object *obj)
> >>>>> VirtIONet *n = VIRTIO_NET(obj);
> >>>>> /*
> >>>>>- * The default config_size is sizeof(struct virtio_net_config).
> >>>>>- * Can be overriden with virtio_net_set_config_size.
> >>>>>+ * The config_size must be set later with
> >>>>>virtio_net_set_config_size.
> >>>>> */
> >>>>>- n->config_size = sizeof(struct virtio_net_config);
> >>>>>+ n->config_size = (size_t)-1;
> >>>>> }
> >>>>> static Property virtio_net_properties[] = {
