diff mbox

[01/23] ppc/pnv: introduce a new intc_create() operation to the chip model

Message ID 20180622042437.14259-1-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show

Commit Message

David Gibson June 22, 2018, 4:24 a.m. UTC 
From: Cédric Le Goater <clg@kaod.org>

On Power9, the thread interrupt presenter has a different type and is
linked to the chip owning the cores.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv.c         | 21 +++++++++++++++++++--
 hw/ppc/pnv_core.c    | 18 +++++++++---------
 include/hw/ppc/pnv.h |  1 +
 3 files changed, 29 insertions(+), 11 deletions(-)

Comments

Greg Kurz June 22, 2018, 9:44 a.m. UTC | #1 
Hi David,

I'm a bit confused by this massive post... it doesn't have any cover letter, but
it contains all the patches you recently pushed to ppc-for-3.0. Was it supposed
to be a pull request ?

Cheers,

--
Greg

On Fri, 22 Jun 2018 14:24:15 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> From: Cédric Le Goater <clg@kaod.org>
> 
> On Power9, the thread interrupt presenter has a different type and is
> linked to the chip owning the cores.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv.c         | 21 +++++++++++++++++++--
>  hw/ppc/pnv_core.c    | 18 +++++++++---------
>  include/hw/ppc/pnv.h |  1 +
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0d2b79f798..c7e127ae97 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -671,6 +671,13 @@ static uint32_t pnv_chip_core_pir_p8(PnvChip *chip, uint32_t core_id)
>      return (chip->chip_id << 7) | (core_id << 3);
>  }
>  
> +static Object *pnv_chip_power8_intc_create(PnvChip *chip, Object *child,
> +                                           Error **errp)
> +{
> +    return icp_create(child, TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
> +                      errp);
> +}
> +
>  /*
>   *    0:48  Reserved - Read as zeroes
>   *   49:52  Node ID
> @@ -686,6 +693,12 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>      return (chip->chip_id << 8) | (core_id << 2);
>  }
>  
> +static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
> +                                           Error **errp)
> +{
> +    return NULL;
> +}
> +
>  /* Allowed core identifiers on a POWER8 Processor Chip :
>   *
>   * <EX0 reserved>
> @@ -721,6 +734,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
> @@ -734,6 +748,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8";
>  }
> @@ -747,6 +762,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
> @@ -760,6 +776,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
> +    k->intc_create = pnv_chip_power9_intc_create;
>      k->xscom_base = 0x00603fc00000000ull;
>      dc->desc = "PowerNV Chip POWER9";
>  }
> @@ -892,8 +909,8 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>          object_property_set_int(OBJECT(pnv_core),
>                                  pcc->core_pir(chip, core_hwid),
>                                  "pir", &error_fatal);
> -        object_property_add_const_link(OBJECT(pnv_core), "xics",
> -                                       qdev_get_machine(), &error_fatal);
> +        object_property_add_const_link(OBJECT(pnv_core), "chip",
> +                                       OBJECT(chip), &error_fatal);
>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>                                   &error_fatal);
>          object_unref(OBJECT(pnv_core));
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index f7cf33f547..a9f129fc2c 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -99,13 +99,14 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> +static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>      int core_pir;
>      int thread_index = 0; /* TODO: TCG supports only one thread */
>      ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
>      Error *local_err = NULL;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
> @@ -113,7 +114,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>          return;
>      }
>  
> -    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> +    cpu->intc = pcc->intc_create(chip, OBJECT(cpu), &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -143,13 +144,12 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      void *obj;
>      int i, j;
>      char name[32];
> -    Object *xi;
> +    Object *chip;
>  
> -    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> -    if (!xi) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> -                   __func__, error_get_pretty(local_err));
> -        return;
> +    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
> +    if (!chip) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "required link 'chip' not found: ");
>      }
>  
>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> @@ -166,7 +166,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
>          if (local_err) {
>              goto err;
>          }
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7..e934e84f55 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -76,6 +76,7 @@ typedef struct PnvChipClass {
>      hwaddr       xscom_base;
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> +    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
David Gibson June 22, 2018, 10:32 a.m. UTC | #2 
On Fri, Jun 22, 2018 at 11:44:39AM +0200, Greg Kurz wrote:
> Hi David,
> 
> I'm a bit confused by this massive post... it doesn't have any cover letter, but
> it contains all the patches you recently pushed to ppc-for-3.0. Was it supposed
> to be a pull request ?

Gah.  Yes, it was.  I'm not sure quite what went wrong there.
Peter Maydell June 26, 2018, 9:07 a.m. UTC | #3 
On 22 June 2018 at 05:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> From: Cédric Le Goater <clg@kaod.org>
>
> On Power9, the thread interrupt presenter has a different type and is
> linked to the chip owning the cores.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv.c         | 21 +++++++++++++++++++--
>  hw/ppc/pnv_core.c    | 18 +++++++++---------
>  include/hw/ppc/pnv.h |  1 +
>  3 files changed, 29 insertions(+), 11 deletions(-)

Hi; Coverity points out a bug (CID 1393617) in this patch
(which is commit d35aefa9ae150a):

> @@ -143,13 +144,12 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      void *obj;
>      int i, j;
>      char name[32];
> -    Object *xi;
> +    Object *chip;
>
> -    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> -    if (!xi) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> -                   __func__, error_get_pretty(local_err));
> -        return;
> +    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
> +    if (!chip) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "required link 'chip' not found: ");
>      }

We check for a NULL 'chip' pointer, but forget the 'return', so
execution will plough on through to the code below and eventually
dereference the NULL pointer and segfault.

thanks
-- PMM
Cédric Le Goater June 26, 2018, 9:31 a.m. UTC | #4 
On 06/26/2018 11:07 AM, Peter Maydell wrote:
> On 22 June 2018 at 05:24, David Gibson <david@gibson.dropbear.id.au> wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> On Power9, the thread interrupt presenter has a different type and is
>> linked to the chip owning the cores.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/pnv.c         | 21 +++++++++++++++++++--
>>  hw/ppc/pnv_core.c    | 18 +++++++++---------
>>  include/hw/ppc/pnv.h |  1 +
>>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> Hi; Coverity points out a bug (CID 1393617) in this patch
> (which is commit d35aefa9ae150a):
> 
>> @@ -143,13 +144,12 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>      void *obj;
>>      int i, j;
>>      char name[32];
>> -    Object *xi;
>> +    Object *chip;
>>
>> -    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
>> -    if (!xi) {
>> -        error_setg(errp, "%s: required link 'xics' not found: %s",
>> -                   __func__, error_get_pretty(local_err));
>> -        return;
>> +    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
>> +    if (!chip) {
>> +        error_propagate(errp, local_err);
>> +        error_prepend(errp, "required link 'chip' not found: ");
>>      }
> 
> We check for a NULL 'chip' pointer, but forget the 'return', so
> execution will plough on through to the code below and eventually
> dereference the NULL pointer and segfault.

arg. My fault. I will send a fix.

Thanks,

C.
diff mbox

Patch

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0d2b79f798..c7e127ae97 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -671,6 +671,13 @@  static uint32_t pnv_chip_core_pir_p8(PnvChip *chip, uint32_t core_id)
     return (chip->chip_id << 7) | (core_id << 3);
 }
 
+static Object *pnv_chip_power8_intc_create(PnvChip *chip, Object *child,
+                                           Error **errp)
+{
+    return icp_create(child, TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
+                      errp);
+}
+
 /*
  *    0:48  Reserved - Read as zeroes
  *   49:52  Node ID
@@ -686,6 +693,12 @@  static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
     return (chip->chip_id << 8) | (core_id << 2);
 }
 
+static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
+                                           Error **errp)
+{
+    return NULL;
+}
+
 /* Allowed core identifiers on a POWER8 Processor Chip :
  *
  * <EX0 reserved>
@@ -721,6 +734,7 @@  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8E";
 }
@@ -734,6 +748,7 @@  static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
@@ -747,6 +762,7 @@  static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
@@ -760,6 +776,7 @@  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
+    k->intc_create = pnv_chip_power9_intc_create;
     k->xscom_base = 0x00603fc00000000ull;
     dc->desc = "PowerNV Chip POWER9";
 }
@@ -892,8 +909,8 @@  static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
         object_property_set_int(OBJECT(pnv_core),
                                 pcc->core_pir(chip, core_hwid),
                                 "pir", &error_fatal);
-        object_property_add_const_link(OBJECT(pnv_core), "xics",
-                                       qdev_get_machine(), &error_fatal);
+        object_property_add_const_link(OBJECT(pnv_core), "chip",
+                                       OBJECT(chip), &error_fatal);
         object_property_set_bool(OBJECT(pnv_core), true, "realized",
                                  &error_fatal);
         object_unref(OBJECT(pnv_core));
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index f7cf33f547..a9f129fc2c 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -99,13 +99,14 @@  static const MemoryRegionOps pnv_core_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
+static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     int core_pir;
     int thread_index = 0; /* TODO: TCG supports only one thread */
     ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
     Error *local_err = NULL;
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
     if (local_err) {
@@ -113,7 +114,7 @@  static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
         return;
     }
 
-    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
+    cpu->intc = pcc->intc_create(chip, OBJECT(cpu), &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -143,13 +144,12 @@  static void pnv_core_realize(DeviceState *dev, Error **errp)
     void *obj;
     int i, j;
     char name[32];
-    Object *xi;
+    Object *chip;
 
-    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
-    if (!xi) {
-        error_setg(errp, "%s: required link 'xics' not found: %s",
-                   __func__, error_get_pretty(local_err));
-        return;
+    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
+    if (!chip) {
+        error_propagate(errp, local_err);
+        error_prepend(errp, "required link 'chip' not found: ");
     }
 
     pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
@@ -166,7 +166,7 @@  static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
+        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
         if (local_err) {
             goto err;
         }
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 90759240a7..e934e84f55 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -76,6 +76,7 @@  typedef struct PnvChipClass {
     hwaddr       xscom_base;
 
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
+    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
 } PnvChipClass;
 
 #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP