diff mbox

[Bug,1187529] Update mappings after PCI bridge live migration or save-restore.

Message ID 1372863856-19860-1-git-send-email-dkoch@terremark.com
State New
Headers show

Commit Message

Don Koch July 3, 2013, 3:04 p.m. UTC 
From: Don Koch <dkoch@verizon.com>

Update mappings for PCI bridge after live migration.

Signed-off-by: Don Koch <dkoch@verizon.com>
---
This fixes bug 1187529: devices on a PCI bridge stop working after migration.

 hw/pci-bridge/pci_bridge_dev.c | 9 +++++++++
 hw/pci/pci_bridge.c            | 2 +-
 include/hw/pci/pci_bridge.h    | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Andreas Färber July 3, 2013, 4:15 p.m. UTC | #1 
Am 03.07.2013 17:04, schrieb Don Koch:
> From: Don Koch <dkoch@verizon.com>
> 
> Update mappings for PCI bridge after live migration.
> 
> Signed-off-by: Don Koch <dkoch@verizon.com>
> ---
> This fixes bug 1187529: devices on a PCI bridge stop working after migration.

Feel free to reference this as LP#1187529 in the commit message if this
is from QEMU's Launchpad bug tracker.

> 
>  hw/pci-bridge/pci_bridge_dev.c | 9 +++++++++
>  hw/pci/pci_bridge.c            | 2 +-
>  include/hw/pci/pci_bridge.h    | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 971b432..9e5062e 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -110,6 +110,14 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>      shpc_reset(dev);
>  }
>  
> +static int pci_bridge_dev_post_load(void *opaque, int ver) {
> +    PCIDevice *d = opaque;
> +    PCIBridge *s = container_of(d, PCIBridge, dev);

Casting should no longer be done that way. It seems there is no
PCI_BRIDGE() cast macro in upstream yet, but it doesn't seem necessary
either. You can just use:

PCIBridge *s = opaque;

for simplicity.

Functional change looks sensible to me.

Regards,
Andreas

> +
> +    pci_bridge_update_mappings(s);
> +    return 0;
> +}
> +
>  static Property pci_bridge_dev_properties[] = {
>                      /* Note: 0 is not a legal chassis number. */
>      DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
> @@ -119,6 +127,7 @@ static Property pci_bridge_dev_properties[] = {
>  
>  static const VMStateDescription pci_bridge_dev_vmstate = {
>      .name = "pci_bridge",
> +    .post_load = pci_bridge_dev_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev),
>          SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev),
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 24be6c5..3897bd8 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -224,7 +224,7 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
>      g_free(w);
>  }
>  
> -static void pci_bridge_update_mappings(PCIBridge *br)
> +void pci_bridge_update_mappings(PCIBridge *br)
>  {
>      PCIBridgeWindows *w = br->windows;
>  
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 1868f7a..1d8f997 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -37,6 +37,7 @@ PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>  pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
>  pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
>  
> +void pci_bridge_update_mappings(PCIBridge *br);
>  void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len);
>  void pci_bridge_disable_base_limit(PCIDevice *dev);
>
Michael S. Tsirkin July 4, 2013, 8:57 a.m. UTC | #2 
On Wed, Jul 03, 2013 at 11:04:16AM -0400, Don Koch wrote:
> From: Don Koch <dkoch@verizon.com>
> 
> Update mappings for PCI bridge after live migration.
> 
> Signed-off-by: Don Koch <dkoch@verizon.com>
> ---
> This fixes bug 1187529: devices on a PCI bridge stop working after migration.

Thanks, this looks good, but any bridge device would
need this fix, won't it?
Could we call this from get_pci_config_device instead?
This way all bridge devices would be fixed.


>  hw/pci-bridge/pci_bridge_dev.c | 9 +++++++++
>  hw/pci/pci_bridge.c            | 2 +-
>  include/hw/pci/pci_bridge.h    | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 971b432..9e5062e 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -110,6 +110,14 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>      shpc_reset(dev);
>  }
>  
> +static int pci_bridge_dev_post_load(void *opaque, int ver) {
> +    PCIDevice *d = opaque;
> +    PCIBridge *s = container_of(d, PCIBridge, dev);
> +
> +    pci_bridge_update_mappings(s);
> +    return 0;
> +}
> +
>  static Property pci_bridge_dev_properties[] = {
>                      /* Note: 0 is not a legal chassis number. */
>      DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
> @@ -119,6 +127,7 @@ static Property pci_bridge_dev_properties[] = {
>  
>  static const VMStateDescription pci_bridge_dev_vmstate = {
>      .name = "pci_bridge",
> +    .post_load = pci_bridge_dev_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev),
>          SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev),
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 24be6c5..3897bd8 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -224,7 +224,7 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
>      g_free(w);
>  }
>  
> -static void pci_bridge_update_mappings(PCIBridge *br)
> +void pci_bridge_update_mappings(PCIBridge *br)
>  {
>      PCIBridgeWindows *w = br->windows;
>  
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 1868f7a..1d8f997 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -37,6 +37,7 @@ PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>  pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
>  pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
>  
> +void pci_bridge_update_mappings(PCIBridge *br);
>  void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len);
>  void pci_bridge_disable_base_limit(PCIDevice *dev);
> -- 
> 1.7.11.7
Don Koch July 8, 2013, 5:02 p.m. UTC | #3 
On 07/03/2013 12:15 PM, Andreas Färber wrote:
> Am 03.07.2013 17:04, schrieb Don Koch:
>> From: Don Koch <dkoch@verizon.com>
>>
>> Update mappings for PCI bridge after live migration.
>>
>> Signed-off-by: Don Koch <dkoch@verizon.com>
>> ---
>> This fixes bug 1187529: devices on a PCI bridge stop working after migration.
> 
> Feel free to reference this as LP#1187529 in the commit message if this
> is from QEMU's Launchpad bug tracker.

OK, will do. (Wasn't aware of this convention.)

>>
>>  hw/pci-bridge/pci_bridge_dev.c | 9 +++++++++
>>  hw/pci/pci_bridge.c            | 2 +-
>>  include/hw/pci/pci_bridge.h    | 1 +
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index 971b432..9e5062e 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -110,6 +110,14 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>      shpc_reset(dev);
>>  }
>>  
>> +static int pci_bridge_dev_post_load(void *opaque, int ver) {
>> +    PCIDevice *d = opaque;
>> +    PCIBridge *s = container_of(d, PCIBridge, dev);
> 
> Casting should no longer be done that way. It seems there is no
> PCI_BRIDGE() cast macro in upstream yet, but it doesn't seem necessary
> either. You can just use:
> 
> PCIBridge *s = opaque;
> 
> for simplicity.

Might I suggest submitting the patch as is (possibly modulo Michael
Tsirkin's suggestions) and later submitting a PCI_BRIDGE macro patch to
"standardize" the casting?

> 
> Functional change looks sensible to me.
> 
> Regards,
> Andreas
> 
>> +
>> +    pci_bridge_update_mappings(s);
>> +    return 0;
>> +}
>> +
>>  static Property pci_bridge_dev_properties[] = {
>>                      /* Note: 0 is not a legal chassis number. */
>>      DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>> @@ -119,6 +127,7 @@ static Property pci_bridge_dev_properties[] = {
>>  
>>  static const VMStateDescription pci_bridge_dev_vmstate = {
>>      .name = "pci_bridge",
>> +    .post_load = pci_bridge_dev_post_load,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev),
>>          SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev),
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 24be6c5..3897bd8 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -224,7 +224,7 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
>>      g_free(w);
>>  }
>>  
>> -static void pci_bridge_update_mappings(PCIBridge *br)
>> +void pci_bridge_update_mappings(PCIBridge *br)
>>  {
>>      PCIBridgeWindows *w = br->windows;
>>  
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index 1868f7a..1d8f997 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -37,6 +37,7 @@ PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>>  pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
>>  pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
>>  
>> +void pci_bridge_update_mappings(PCIBridge *br);
>>  void pci_bridge_write_config(PCIDevice *d,
>>                               uint32_t address, uint32_t val, int len);
>>  void pci_bridge_disable_base_limit(PCIDevice *dev);
>>
> 
>
Don Koch July 8, 2013, 5:30 p.m. UTC | #4 
On 07/04/2013 04:57 AM, Michael S. Tsirkin wrote:
> On Wed, Jul 03, 2013 at 11:04:16AM -0400, Don Koch wrote:
>> From: Don Koch <dkoch@verizon.com>
>>
>> Update mappings for PCI bridge after live migration.
>>
>> Signed-off-by: Don Koch <dkoch@verizon.com>
>> ---
>> This fixes bug 1187529: devices on a PCI bridge stop working after migration.
> Thanks, this looks good, but any bridge device would
> need this fix, won't it?
One would think so.
> Could we call this from get_pci_config_device instead?
Seems a bit odd, but I can't find a better spot (there is no pci_bridge
equivalent).
I'll look into a patch for that (but can't guarantee total testing).
> This way all bridge devices would be fixed.
Well, all that support migration (e.g., not the i82801b11). ;)
>
>>  hw/pci-bridge/pci_bridge_dev.c | 9 +++++++++
>>  hw/pci/pci_bridge.c            | 2 +-
>>  include/hw/pci/pci_bridge.h    | 1 +
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index 971b432..9e5062e 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -110,6 +110,14 @@ static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>      shpc_reset(dev);
>>  }
>>  
>> +static int pci_bridge_dev_post_load(void *opaque, int ver) {
>> +    PCIDevice *d = opaque;
>> +    PCIBridge *s = container_of(d, PCIBridge, dev);
>> +
>> +    pci_bridge_update_mappings(s);
>> +    return 0;
>> +}
>> +
>>  static Property pci_bridge_dev_properties[] = {
>>                      /* Note: 0 is not a legal chassis number. */
>>      DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>> @@ -119,6 +127,7 @@ static Property pci_bridge_dev_properties[] = {
>>  
>>  static const VMStateDescription pci_bridge_dev_vmstate = {
>>      .name = "pci_bridge",
>> +    .post_load = pci_bridge_dev_post_load,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev),
>>          SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev),
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 24be6c5..3897bd8 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -224,7 +224,7 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
>>      g_free(w);
>>  }
>>  
>> -static void pci_bridge_update_mappings(PCIBridge *br)
>> +void pci_bridge_update_mappings(PCIBridge *br)
>>  {
>>      PCIBridgeWindows *w = br->windows;
>>  
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index 1868f7a..1d8f997 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -37,6 +37,7 @@ PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>>  pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
>>  pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
>>  
>> +void pci_bridge_update_mappings(PCIBridge *br);
>>  void pci_bridge_write_config(PCIDevice *d,
>>                               uint32_t address, uint32_t val, int len);
>>  void pci_bridge_disable_base_limit(PCIDevice *dev);
>> -- 
>> 1.7.11.7
diff mbox

Patch

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 971b432..9e5062e 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -110,6 +110,14 @@  static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
     shpc_reset(dev);
 }
 
+static int pci_bridge_dev_post_load(void *opaque, int ver) {
+    PCIDevice *d = opaque;
+    PCIBridge *s = container_of(d, PCIBridge, dev);
+
+    pci_bridge_update_mappings(s);
+    return 0;
+}
+
 static Property pci_bridge_dev_properties[] = {
                     /* Note: 0 is not a legal chassis number. */
     DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
@@ -119,6 +127,7 @@  static Property pci_bridge_dev_properties[] = {
 
 static const VMStateDescription pci_bridge_dev_vmstate = {
     .name = "pci_bridge",
+    .post_load = pci_bridge_dev_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev),
         SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev),
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 24be6c5..3897bd8 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -224,7 +224,7 @@  static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
     g_free(w);
 }
 
-static void pci_bridge_update_mappings(PCIBridge *br)
+void pci_bridge_update_mappings(PCIBridge *br)
 {
     PCIBridgeWindows *w = br->windows;
 
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 1868f7a..1d8f997 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -37,6 +37,7 @@  PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
 pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
 pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
 
+void pci_bridge_update_mappings(PCIBridge *br);
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len);
 void pci_bridge_disable_base_limit(PCIDevice *dev);