| Message ID | 20130507102223.GA16878@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, I think it is not a good idea, as we wanted to make VirtIODevice hot-pluggable for virtio-mmio. Maybe we can use a callback which is called by virtio-bus before plugging, to ensure that this config size is computed? On 07/05/2013 12:22, Michael S. Tsirkin wrote: > 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 <mst@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[] = {
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. > 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? > On 07/05/2013 12:22, Michael S. Tsirkin wrote: > >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 <mst@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[] = {
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? > >> 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. 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. >> On 07/05/2013 12:22, Michael S. Tsirkin wrote: >>> 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 <mst@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[] = {
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. > > > >>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. > 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 <mst@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[] = {
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. 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? > >> 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 <mst@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[] = {
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 <mst@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[] = {
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[] = {
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 <mst@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(-)