diff mbox

[1/5] spapr: ensure device trees are always associated with DRC

Message ID 1460752385-13259-2-git-send-email-duanj@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jianjun Duan April 15, 2016, 8:33 p.m. UTC 
There are possible racing situations involving hotplug events and
guest migration. For cases where a hotplug event is migrated, or
the guest is in the process of fetching device tree at the time of
migration, we need to ensure the device tree is created and
associated with the corresponding DRC for devices that were
hotplugged on the source, but 'coldplugged' on the target.

Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c     | 16 ++++++----------
 hw/ppc/spapr_pci.c | 12 +++++-------
 2 files changed, 11 insertions(+), 17 deletions(-)

Comments

David Gibson April 20, 2016, 4:30 a.m. UTC | #1 
On Fri, Apr 15, 2016 at 01:33:01PM -0700, Jianjun Duan wrote:
> There are possible racing situations involving hotplug events and
> guest migration. For cases where a hotplug event is migrated, or
> the guest is in the process of fetching device tree at the time of
> migration, we need to ensure the device tree is created and
> associated with the corresponding DRC for devices that were
> hotplugged on the source, but 'coldplugged' on the target.
> 
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>

This seems fairly sensible - should be harmless and a bit cleaner
whether or not we strictly speaking need it.

However, it is likely to conflict with some of the device tree
construction cleanups I have in my pipeline, so it may well need
rework for that.

> ---
>  hw/ppc/spapr.c     | 16 ++++++----------
>  hw/ppc/spapr_pci.c | 12 +++++-------
>  2 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index feaab08..af4745c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2132,15 +2132,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
>      int i, fdt_offset, fdt_size;
>      void *fdt;
>  
> -    /*
> -     * Check for DRC connectors and send hotplug notification to the
> -     * guest only in case of hotplugged memory. This allows cold plugged
> -     * memory to be specified at boot time.
> -     */
> -    if (!dev->hotplugged) {
> -        return;
> -    }
> -
>      for (i = 0; i < nr_lmbs; i++) {
>          drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                  addr/SPAPR_MEMORY_BLOCK_SIZE);
> @@ -2154,7 +2145,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
>          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
> -    spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> +    /* send hotplug notification to the
> +     * guest only in case of hotplugged memory
> +     */
> +    if (dev->hotplugged) {
> +       spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
> +    }
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 8c20d34..b179e42 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1092,13 +1092,11 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>          spapr_tce_set_need_vfio(tcet, true);
>      }
>  
> -    if (dev->hotplugged) {
> -        fdt = create_device_tree(&fdt_size);
> -        fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> -        if (!fdt_start_offset) {
> -            error_setg(errp, "Failed to create pci child device tree node");
> -            goto out;
> -        }
> +    fdt = create_device_tree(&fdt_size);
> +    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> +    if (!fdt_start_offset) {
> +        error_setg(errp, "Failed to create pci child device tree node");
> +        goto out;
>      }
>  
>      drck->attach(drc, DEVICE(pdev),
Jianjun Duan April 21, 2016, 4:53 p.m. UTC | #2 
On 04/19/2016 09:30 PM, David Gibson wrote:
> On Fri, Apr 15, 2016 at 01:33:01PM -0700, Jianjun Duan wrote:
>> There are possible racing situations involving hotplug events and
>> guest migration. For cases where a hotplug event is migrated, or
>> the guest is in the process of fetching device tree at the time of
>> migration, we need to ensure the device tree is created and
>> associated with the corresponding DRC for devices that were
>> hotplugged on the source, but 'coldplugged' on the target.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> This seems fairly sensible - should be harmless and a bit cleaner
> whether or not we strictly speaking need it.
>
> However, it is likely to conflict with some of the device tree
> construction cleanups I have in my pipeline, so it may well need
> rework for that.
Sure. It will need rework for CPU hotplug too.
>> ---
>>   hw/ppc/spapr.c     | 16 ++++++----------
>>   hw/ppc/spapr_pci.c | 12 +++++-------
>>   2 files changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index feaab08..af4745c 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2132,15 +2132,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
>>       int i, fdt_offset, fdt_size;
>>       void *fdt;
>>   
>> -    /*
>> -     * Check for DRC connectors and send hotplug notification to the
>> -     * guest only in case of hotplugged memory. This allows cold plugged
>> -     * memory to be specified at boot time.
>> -     */
>> -    if (!dev->hotplugged) {
>> -        return;
>> -    }
>> -
>>       for (i = 0; i < nr_lmbs; i++) {
>>           drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>>                   addr/SPAPR_MEMORY_BLOCK_SIZE);
>> @@ -2154,7 +2145,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
>>           drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
>>           addr += SPAPR_MEMORY_BLOCK_SIZE;
>>       }
>> -    spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
>> +    /* send hotplug notification to the
>> +     * guest only in case of hotplugged memory
>> +     */
>> +    if (dev->hotplugged) {
>> +       spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
>> +    }
>>   }
>>   
>>   static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 8c20d34..b179e42 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1092,13 +1092,11 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>>           spapr_tce_set_need_vfio(tcet, true);
>>       }
>>   
>> -    if (dev->hotplugged) {
>> -        fdt = create_device_tree(&fdt_size);
>> -        fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
>> -        if (!fdt_start_offset) {
>> -            error_setg(errp, "Failed to create pci child device tree node");
>> -            goto out;
>> -        }
>> +    fdt = create_device_tree(&fdt_size);
>> +    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
>> +    if (!fdt_start_offset) {
>> +        error_setg(errp, "Failed to create pci child device tree node");
>> +        goto out;
>>       }
>>   
>>       drck->attach(drc, DEVICE(pdev),
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index feaab08..af4745c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2132,15 +2132,6 @@  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
     int i, fdt_offset, fdt_size;
     void *fdt;
 
-    /*
-     * Check for DRC connectors and send hotplug notification to the
-     * guest only in case of hotplugged memory. This allows cold plugged
-     * memory to be specified at boot time.
-     */
-    if (!dev->hotplugged) {
-        return;
-    }
-
     for (i = 0; i < nr_lmbs; i++) {
         drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
                 addr/SPAPR_MEMORY_BLOCK_SIZE);
@@ -2154,7 +2145,12 @@  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
         drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
-    spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
+    /* send hotplug notification to the
+     * guest only in case of hotplugged memory
+     */
+    if (dev->hotplugged) {
+       spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
+    }
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8c20d34..b179e42 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1092,13 +1092,11 @@  static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
         spapr_tce_set_need_vfio(tcet, true);
     }
 
-    if (dev->hotplugged) {
-        fdt = create_device_tree(&fdt_size);
-        fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
-        if (!fdt_start_offset) {
-            error_setg(errp, "Failed to create pci child device tree node");
-            goto out;
-        }
+    fdt = create_device_tree(&fdt_size);
+    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
+    if (!fdt_start_offset) {
+        error_setg(errp, "Failed to create pci child device tree node");
+        goto out;
     }
 
     drck->attach(drc, DEVICE(pdev),