| Message ID | 51950139.5000209@greensocs.com |
|---|---|
| State | New |
| Headers | show |
Il 16/05/2013 17:54, KONRAD Frédéric ha scritto: > I think this can do the job, any better idea? > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index d5257ed..e033b53 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -43,7 +43,6 @@ > #endif > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > -static char *pcibus_get_dev_path(DeviceState *dev); > static char *pcibus_get_fw_dev_path(DeviceState *dev); > static int pcibus_reset(BusState *qbus); > > @@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev) > return g_strdup(path); > } > > -static char *pcibus_get_dev_path(DeviceState *dev) > +char *pcibus_get_dev_path(DeviceState *dev) > { > PCIDevice *d = container_of(dev, PCIDevice, qdev); > PCIDevice *t; > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 70d2c6b..0241223 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState > *bus, VirtIOPCIProxy *dev) > qbus->allow_hotplug = 1; > } > > +static char *virtio_pci_bus_get_dev_path(DeviceState *dev) > +{ > + BusState *bus = qdev_get_parent_bus(dev); > + DeviceState *proxy = DEVICE(bus->parent); > + return g_strdup(pcibus_get_dev_path(proxy)); You do not need to export pcibus_get_dev_path. This should just return qdev_get_dev_path(proxy) and should be in TYPE_VIRTIO_BUS, not in the PCI-specific subclass. (The g_strdup is not needed, either). Paolo > +} > + > static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) > { > BusClass *bus_class = BUS_CLASS(klass); > VirtioBusClass *k = VIRTIO_BUS_CLASS(klass); > bus_class->max_dev = 1; > + bus_class->get_dev_path = virtio_pci_bus_get_dev_path; > k->notify = virtio_pci_notify; > k->save_config = virtio_pci_save_config; > k->load_config = virtio_pci_load_config; > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 8d075ab..fb5723c 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -708,6 +708,8 @@ static inline void pci_dma_sglist_init(QEMUSGList > *qsg, PCIDevice *dev, > > extern const VMStateDescription vmstate_pci_device; > > +char *pcibus_get_dev_path(DeviceState *dev); > + > #define VMSTATE_PCI_DEVICE(_field, _state) { \ > .name = (stringify(_field)), \ > .size = sizeof(PCIDevice), \
KONRAD Frédéric <fred.konrad@greensocs.com> writes: > On 16/05/2013 16:51, Paolo Bonzini wrote: >> Il 16/05/2013 16:21, mdroth ha scritto: >>> commit e37da3945fa2fde161e1b217f937fc318c4b7639 >>> Author: KONRAD Frederic <fred.konrad@greensocs.com> >>> Date: Thu Apr 11 16:29:58 2013 +0200 >>> >>> virtio-net-pci: switch to the new API. >>> >>> Here the virtio-net-pci is modified for the new API. The device >>> virtio-net-pci extends virtio-pci. It creates and connects a >>> virtio-net-device during the init. The properties are not changed. >>> >>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>> Message-id: 1365690602-22729-4-git-send-email-fred.konrad@greensocs.com >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>> >>> And if we roll that back, we have similar failures for virtio-blk, and most >>> likely the other virtio devices touched by the refactoring. >>> >>> The issue seems to be a change the way section id strings are generated in >>> vmstate_register(). In v1.4.x we had: >>> >>> se->instance_id: 0, se->idstr: 0000:00:03.0/virtio-net >>> >>> In v1.5.0-rc2 we have: >>> >>> se->instance_id: 0, se->idstr: virtio-net >>> >>> This seems to be due to the fact that these devices now sit on a >>> TYPE_VIRTIO_BUS that has no implementation of TYPE_BUS's get_dev_path() >>> interface, which is what savevm uses to calculate the id prefix for >>> se->idstr. >>> >>> Prior to the refactoring, the device sat on a TYPE_PCI_BUS which used >>> pcibus_get_dev_path() to calculate this. >>> >>> I'm not sure what the best fix is for this. I looking at implementing >>> get_dev_path() for TYPE_VIRTIO_BUS, but to maintain migration >>> compatibility we'd end up baking in PCI-specific stuff which from what >>> I gather is exactly what we were trying to avoid there. >> I think get_dev_path for TYPE_VIRTIO_BUS could simply forward to the >> parent device's parent bus. >> >> Paolo > > I think this can do the job, any better idea? Monkey patching is a little ugly but it is a concise way of fixing the problem. Mike, can you confirm the patch? Fred, can you add a Signed-off-by and send as a top-level? Regards, Anthony Liguori > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index d5257ed..e033b53 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -43,7 +43,6 @@ > #endif > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > -static char *pcibus_get_dev_path(DeviceState *dev); > static char *pcibus_get_fw_dev_path(DeviceState *dev); > static int pcibus_reset(BusState *qbus); > > @@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev) > return g_strdup(path); > } > > -static char *pcibus_get_dev_path(DeviceState *dev) > +char *pcibus_get_dev_path(DeviceState *dev) > { > PCIDevice *d = container_of(dev, PCIDevice, qdev); > PCIDevice *t; > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 70d2c6b..0241223 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState > *bus, VirtIOPCIProxy *dev) > qbus->allow_hotplug = 1; > } > > +static char *virtio_pci_bus_get_dev_path(DeviceState *dev) > +{ > + BusState *bus = qdev_get_parent_bus(dev); > + DeviceState *proxy = DEVICE(bus->parent); > + return g_strdup(pcibus_get_dev_path(proxy)); > +} > + > static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) > { > BusClass *bus_class = BUS_CLASS(klass); > VirtioBusClass *k = VIRTIO_BUS_CLASS(klass); > bus_class->max_dev = 1; > + bus_class->get_dev_path = virtio_pci_bus_get_dev_path; > k->notify = virtio_pci_notify; > k->save_config = virtio_pci_save_config; > k->load_config = virtio_pci_load_config; > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 8d075ab..fb5723c 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -708,6 +708,8 @@ static inline void pci_dma_sglist_init(QEMUSGList > *qsg, PCIDevice *dev, > > extern const VMStateDescription vmstate_pci_device; > > +char *pcibus_get_dev_path(DeviceState *dev); > + > #define VMSTATE_PCI_DEVICE(_field, _state) { \ > .name = (stringify(_field)), \ > .size = sizeof(PCIDevice), \ > -- > 1.7.11.7
On 16/05/2013 18:33, Anthony Liguori wrote: > KONRAD Frédéric <fred.konrad@greensocs.com> writes: > >> On 16/05/2013 16:51, Paolo Bonzini wrote: >>> Il 16/05/2013 16:21, mdroth ha scritto: >>>> commit e37da3945fa2fde161e1b217f937fc318c4b7639 >>>> Author: KONRAD Frederic <fred.konrad@greensocs.com> >>>> Date: Thu Apr 11 16:29:58 2013 +0200 >>>> >>>> virtio-net-pci: switch to the new API. >>>> >>>> Here the virtio-net-pci is modified for the new API. The device >>>> virtio-net-pci extends virtio-pci. It creates and connects a >>>> virtio-net-device during the init. The properties are not changed. >>>> >>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>>> Message-id: 1365690602-22729-4-git-send-email-fred.konrad@greensocs.com >>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>>> >>>> And if we roll that back, we have similar failures for virtio-blk, and most >>>> likely the other virtio devices touched by the refactoring. >>>> >>>> The issue seems to be a change the way section id strings are generated in >>>> vmstate_register(). In v1.4.x we had: >>>> >>>> se->instance_id: 0, se->idstr: 0000:00:03.0/virtio-net >>>> >>>> In v1.5.0-rc2 we have: >>>> >>>> se->instance_id: 0, se->idstr: virtio-net >>>> >>>> This seems to be due to the fact that these devices now sit on a >>>> TYPE_VIRTIO_BUS that has no implementation of TYPE_BUS's get_dev_path() >>>> interface, which is what savevm uses to calculate the id prefix for >>>> se->idstr. >>>> >>>> Prior to the refactoring, the device sat on a TYPE_PCI_BUS which used >>>> pcibus_get_dev_path() to calculate this. >>>> >>>> I'm not sure what the best fix is for this. I looking at implementing >>>> get_dev_path() for TYPE_VIRTIO_BUS, but to maintain migration >>>> compatibility we'd end up baking in PCI-specific stuff which from what >>>> I gather is exactly what we were trying to avoid there. >>> I think get_dev_path for TYPE_VIRTIO_BUS could simply forward to the >>> parent device's parent bus. >>> >>> Paolo >> I think this can do the job, any better idea? > Monkey patching is a little ugly but it is a concise way of fixing the > problem. > > Mike, can you confirm the patch? > > Fred, can you add a Signed-off-by and send as a top-level? > > Regards, > > Anthony Liguori I just send a better way. > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index d5257ed..e033b53 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -43,7 +43,6 @@ >> #endif >> >> static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); >> -static char *pcibus_get_dev_path(DeviceState *dev); >> static char *pcibus_get_fw_dev_path(DeviceState *dev); >> static int pcibus_reset(BusState *qbus); >> >> @@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev) >> return g_strdup(path); >> } >> >> -static char *pcibus_get_dev_path(DeviceState *dev) >> +char *pcibus_get_dev_path(DeviceState *dev) >> { >> PCIDevice *d = container_of(dev, PCIDevice, qdev); >> PCIDevice *t; >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 70d2c6b..0241223 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState >> *bus, VirtIOPCIProxy *dev) >> qbus->allow_hotplug = 1; >> } >> >> +static char *virtio_pci_bus_get_dev_path(DeviceState *dev) >> +{ >> + BusState *bus = qdev_get_parent_bus(dev); >> + DeviceState *proxy = DEVICE(bus->parent); >> + return g_strdup(pcibus_get_dev_path(proxy)); >> +} >> + >> static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) >> { >> BusClass *bus_class = BUS_CLASS(klass); >> VirtioBusClass *k = VIRTIO_BUS_CLASS(klass); >> bus_class->max_dev = 1; >> + bus_class->get_dev_path = virtio_pci_bus_get_dev_path; >> k->notify = virtio_pci_notify; >> k->save_config = virtio_pci_save_config; >> k->load_config = virtio_pci_load_config; >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 8d075ab..fb5723c 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -708,6 +708,8 @@ static inline void pci_dma_sglist_init(QEMUSGList >> *qsg, PCIDevice *dev, >> >> extern const VMStateDescription vmstate_pci_device; >> >> +char *pcibus_get_dev_path(DeviceState *dev); >> + >> #define VMSTATE_PCI_DEVICE(_field, _state) { \ >> .name = (stringify(_field)), \ >> .size = sizeof(PCIDevice), \ >> -- >> 1.7.11.7
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index d5257ed..e033b53 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -43,7 +43,6 @@ #endif static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); -static char *pcibus_get_dev_path(DeviceState *dev); static char *pcibus_get_fw_dev_path(DeviceState *dev); static int pcibus_reset(BusState *qbus); @@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev) return g_strdup(path); } -static char *pcibus_get_dev_path(DeviceState *dev) +char *pcibus_get_dev_path(DeviceState *dev) { PCIDevice *d = container_of(dev, PCIDevice, qdev); PCIDevice *t; diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 70d2c6b..0241223 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState *bus, VirtIOPCIProxy *dev) qbus->allow_hotplug = 1; } +static char *virtio_pci_bus_get_dev_path(DeviceState *dev) +{ + BusState *bus = qdev_get_parent_bus(dev); + DeviceState *proxy = DEVICE(bus->parent); + return g_strdup(pcibus_get_dev_path(proxy)); +} + static void virtio_pci_bus_class_init(ObjectClass *klass, void *data) { BusClass *bus_class = BUS_CLASS(klass); VirtioBusClass *k = VIRTIO_BUS_CLASS(klass); bus_class->max_dev = 1; + bus_class->get_dev_path = virtio_pci_bus_get_dev_path; k->notify = virtio_pci_notify; k->save_config = virtio_pci_save_config; k->load_config = virtio_pci_load_config; diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 8d075ab..fb5723c 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -708,6 +708,8 @@ static inline void pci_dma_sglist_init(QEMUSGList *qsg, PCIDevice *dev, extern const VMStateDescription vmstate_pci_device; +char *pcibus_get_dev_path(DeviceState *dev); + #define VMSTATE_PCI_DEVICE(_field, _state) { \ .name = (stringify(_field)), \