[Qemu-devel] [PATCH v2 0/4] ppc/pnv: XSCOM fixes and unit tests

[Qemu-devel] [PATCH v2 0/4] ppc/pnv: XSCOM fixes and unit tests

[Cédric Le Goater]

Hello,

Here is a little serie adding some fixes for the XSCOM registers of
the POWER9 cores and a unit test. 

Changes since v1 :

 - fixed pnv_xscom_addr() for 32bit host systems
 - replace hweight_long() by ctpop64()

Tested on a 32-bit LE system and on 64-bit LE and BE systems.

Thanks,

C.

Cédric Le Goater (3):
  ppc/pnv: add a 'xscom_core_base' field to PnvChipClass
  ppc/pnv: fix xscom address translation for POWER9
  tests: add XSCOM tests for the PowerNV machine

David Gibson (1):
  ppc/pnv: Fix fatal bug on 32-bit hosts

 hw/ppc/pnv.c               |  10 +++-
 hw/ppc/pnv_xscom.c         |   8 +--
 include/hw/ppc/pnv.h       |   1 +
 include/hw/ppc/pnv_xscom.h |   5 +-
 tests/Makefile.include     |   1 +
 tests/pnv-xscom-test.c     | 140 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 156 insertions(+), 9 deletions(-)
 create mode 100644 tests/pnv-xscom-test.c

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] ppc/pnv: add a 'xscom_core_base' field to PnvChipClass

[Cédric Le Goater]

The XSCOM addresses for the core registers are encoded in a slightly
different way on POWER8 and POWER9.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c               | 8 +++++++-
 include/hw/ppc/pnv.h       | 1 +
 include/hw/ppc/pnv_xscom.h | 5 ++---
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 6af34241f248..e7779581545d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -521,6 +521,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->xscom_base = 0x003fc0000000000ull;
+    k->xscom_core_base = 0x10000000ull;
     dc->desc = "PowerNV Chip POWER8E";
 }
 
@@ -542,6 +543,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->xscom_base = 0x003fc0000000000ull;
+    k->xscom_core_base = 0x10000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
 
@@ -563,6 +565,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->xscom_base = 0x003fc0000000000ull;
+    k->xscom_core_base = 0x10000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
 
@@ -584,6 +587,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
     k->xscom_base = 0x00603fc00000000ull;
+    k->xscom_core_base = 0x0ull;
     dc->desc = "PowerNV Chip POWER9";
 }
 
@@ -691,7 +695,9 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
         object_unref(OBJECT(pnv_core));
 
         /* Each core has an XSCOM MMIO region */
-        pnv_xscom_add_subregion(chip, PNV_XSCOM_EX_CORE_BASE(core_hwid),
+        pnv_xscom_add_subregion(chip,
+                                PNV_XSCOM_EX_CORE_BASE(pcc->xscom_core_base,
+                                                       core_hwid),
                                 &PNV_CORE(pnv_core)->xscom_regs);
         i++;
     }
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 7bee658733db..df98a72006e4 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -69,6 +69,7 @@ typedef struct PnvChipClass {
     uint64_t     cores_mask;
 
     hwaddr       xscom_base;
+    hwaddr       xscom_core_base;
 
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
 } PnvChipClass;
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index 41a5127a1907..0faa1847bf13 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -40,7 +40,7 @@ typedef struct PnvXScomInterfaceClass {
 } PnvXScomInterfaceClass;
 
 /*
- * Layout of the XSCOM PCB addresses of EX core 1
+ * Layout of the XSCOM PCB addresses of EX core 1 (POWER 8)
  *
  *   GPIO        0x1100xxxx
  *   SCOM        0x1101xxxx
@@ -54,8 +54,7 @@ typedef struct PnvXScomInterfaceClass {
  *   PCB SLAVE   0x110Fxxxx
  */
 
-#define PNV_XSCOM_EX_BASE         0x10000000
-#define PNV_XSCOM_EX_CORE_BASE(i) (PNV_XSCOM_EX_BASE | (((uint64_t)i) << 24))
+#define PNV_XSCOM_EX_CORE_BASE(base, i) (base | (((uint64_t)i) << 24))
 #define PNV_XSCOM_EX_CORE_SIZE    0x100000
 
 #define PNV_XSCOM_LPC_BASE        0xb0020
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] ppc/pnv: fix xscom address translation for POWER9

[Cédric Le Goater]

High addresses can overflow the uint32_t pcba variable after the 8byte
shift.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_xscom.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index f46646141a96..8da271872f59 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -124,8 +124,8 @@ static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
         goto complete;
     }
 
-    val = address_space_ldq(&chip->xscom_as, pcba << 3, MEMTXATTRS_UNSPECIFIED,
-                            &result);
+    val = address_space_ldq(&chip->xscom_as, (uint64_t) pcba << 3,
+                            MEMTXATTRS_UNSPECIFIED, &result);
     if (result != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR, "XSCOM read failed at @0x%"
                       HWADDR_PRIx " pcba=0x%08x\n", addr, pcba);
@@ -150,8 +150,8 @@ static void xscom_write(void *opaque, hwaddr addr, uint64_t val,
         goto complete;
     }
 
-    address_space_stq(&chip->xscom_as, pcba << 3, val, MEMTXATTRS_UNSPECIFIED,
-                      &result);
+    address_space_stq(&chip->xscom_as, (uint64_t) pcba << 3, val,
+                      MEMTXATTRS_UNSPECIFIED, &result);
     if (result != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR, "XSCOM write failed at @0x%"
                       HWADDR_PRIx " pcba=0x%08x data=0x%" PRIx64 "\n",
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] ppc/pnv: Fix fatal bug on 32-bit hosts

[Cédric Le Goater]

From: David Gibson <david@gibson.dropbear.id.au>

If the pnv machine type is compiled on a 32-bit host, the unsigned long
(host) type is 32-bit.  This means that the hweight_long() used to
calculate the number of allowed cores only considers the low 32 bits of
the cores_mask variable, and can thus return 0 in some circumstances.

This corrects the bug.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Suggested-by: Richard Henderson <rth@twiddle.net>
[clg: replaced hweight_long() by ctpop64() ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e7779581545d..9df7b25315af 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -620,7 +620,7 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
     chip->cores_mask &= pcc->cores_mask;
 
     /* now that we have a sane layout, let check the number of cores */
-    cores_max = hweight_long(chip->cores_mask);
+    cores_max = ctpop64(chip->cores_mask);
     if (chip->nr_cores > cores_max) {
         error_setg(errp, "warning: too many cores for chip ! Limit is %d",
                    cores_max);
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] tests: add XSCOM tests for the PowerNV machine

[Cédric Le Goater]

Add a couple of tests on the XSCOM bus of the PowerNV machine for the
the POWER8 and POWER9 CPUs. The first tests reads the CFAM identifier
of the chip. The second test goes further in the XSCOM address space
and reaches the cores to read their DTS registers.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Changes since v1:

 - fixed pnv_xscom_addr() for 32bit host systems

 tests/Makefile.include |   1 +
 tests/pnv-xscom-test.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 tests/pnv-xscom-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index de516341fd44..90c9ad9ac6e1 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -270,6 +270,7 @@ gcov-files-ppc64-y = ppc64-softmmu/hw/ppc/spapr_pci.c
 check-qtest-ppc64-y += tests/endianness-test$(EXESUF)
 check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
 check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
+check-qtest-ppc64-y += tests/pnv-xscom-test$(EXESUF)
 check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
 check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
 check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
diff --git a/tests/pnv-xscom-test.c b/tests/pnv-xscom-test.c
new file mode 100644
index 000000000000..f9294e677382
--- /dev/null
+++ b/tests/pnv-xscom-test.c
@@ -0,0 +1,140 @@
+/*
+ * QTest testcase for PowerNV XSCOM bus
+ *
+ * Copyright (c) 2016, IBM Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+
+#include "libqtest.h"
+
+typedef enum PnvChipType {
+    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
+    PNV_CHIP_POWER8,      /* AKA Venice */
+    PNV_CHIP_POWER8NVL,   /* AKA Naples */
+    PNV_CHIP_POWER9,      /* AKA Nimbus */
+} PnvChipType;
+
+typedef struct PnvChip {
+    PnvChipType chip_type;
+    const char *cpu_model;
+    uint64_t    xscom_base;
+    uint64_t    xscom_core_base;
+    uint64_t    cfam_id;
+    uint32_t    first_core;
+} PnvChip;
+
+static const PnvChip pnv_chips[] = {
+    {
+        .chip_type  = PNV_CHIP_POWER8,
+        .cpu_model  = "POWER8",
+        .xscom_base = 0x0003fc0000000000ull,
+        .xscom_core_base = 0x10000000ull,
+        .cfam_id    = 0x220ea04980000000ull,
+        .first_core = 0x1,
+    }, {
+        .chip_type  = PNV_CHIP_POWER8NVL,
+        .cpu_model  = "POWER8NVL",
+        .xscom_base = 0x0003fc0000000000ull,
+        .xscom_core_base = 0x10000000ull,
+        .cfam_id    = 0x120d304980000000ull,
+        .first_core = 0x1,
+    }, {
+        .chip_type  = PNV_CHIP_POWER9,
+        .cpu_model  = "POWER9",
+        .xscom_base = 0x000603fc00000000ull,
+        .xscom_core_base = 0x0ull,
+        .cfam_id    = 0x100d104980000000ull,
+        .first_core = 0x20,
+    },
+};
+
+static uint64_t pnv_xscom_addr(const PnvChip *chip, uint32_t pcba)
+{
+    uint64_t addr = chip->xscom_base;
+
+    if (chip->chip_type == PNV_CHIP_POWER9) {
+         addr |= ((uint64_t) pcba << 3);
+    } else {
+         addr |= (((uint64_t) pcba << 4) & ~0xffull) |
+            (((uint64_t) pcba << 3) & 0x78);
+    }
+    return addr;
+}
+
+static uint64_t pnv_xscom_read(const PnvChip *chip, uint32_t pcba)
+{
+    return readq(pnv_xscom_addr(chip, pcba));
+}
+
+static void test_xscom_cfam_id(const PnvChip *chip)
+{
+    uint64_t f000f = pnv_xscom_read(chip, 0xf000f);
+
+    g_assert_cmphex(f000f, ==, chip->cfam_id);
+}
+
+static void test_cfam_id(const void *data)
+{
+    char *args;
+    const PnvChip *chip = data;
+
+    args = g_strdup_printf("-M powernv,accel=tcg -cpu %s", chip->cpu_model);
+
+    qtest_start(args);
+    test_xscom_cfam_id(chip);
+    qtest_quit(global_qtest);
+
+    g_free(args);
+}
+
+#define PNV_XSCOM_EX_CORE_BASE(chip, i)                 \
+    ((chip)->xscom_core_base | (((uint64_t)i) << 24))
+#define PNV_XSCOM_EX_DTS_RESULT0     0x50000
+
+static void test_xscom_core(const PnvChip *chip)
+{
+    uint32_t first_core_dts0 =
+        PNV_XSCOM_EX_CORE_BASE(chip, chip->first_core) |
+        PNV_XSCOM_EX_DTS_RESULT0;
+    uint64_t dts0 = pnv_xscom_read(chip, first_core_dts0);
+
+    g_assert_cmphex(dts0, ==, 0x26f024f023f0000ull);
+}
+
+static void test_core(const void *data)
+{
+    char *args;
+    const PnvChip *chip = data;
+
+    args = g_strdup_printf("-M powernv,accel=tcg -cpu %s", chip->cpu_model);
+
+    qtest_start(args);
+    test_xscom_core(chip);
+    qtest_quit(global_qtest);
+
+    g_free(args);
+}
+
+static void add_test(const char *name, void (*test)(const void *data))
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pnv_chips); i++) {
+        char *tname = g_strdup_printf("pnv-xscom/%s/%s", name,
+                                      pnv_chips[i].cpu_model);
+        qtest_add_data_func(tname, &pnv_chips[i], test);
+        g_free(tname);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    add_test("cfam_id", test_cfam_id);
+    add_test("core", test_core);
+    return g_test_run();
+}
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/4] ppc/pnv: XSCOM fixes and unit tests

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v2 0/4]  ppc/pnv: XSCOM fixes and unit tests
Message-id: 1479114778-3881-1-git-send-email-clg@kaod.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1479114778-3881-1-git-send-email-clg@kaod.org -> patchew/1479114778-3881-1-git-send-email-clg@kaod.org
Switched to a new branch 'test'
d3d45dc tests: add XSCOM tests for the PowerNV machine
495b638 ppc/pnv: Fix fatal bug on 32-bit hosts
662eee5 ppc/pnv: fix xscom address translation for POWER9
1a6e1d7 ppc/pnv: add a 'xscom_core_base' field to PnvChipClass

=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/4: ...
fatal: unrecognized argument: --no-patch
Checking PATCH 2/4: ...
fatal: unrecognized argument: --no-patch
Checking PATCH 3/4: ...
fatal: unrecognized argument: --no-patch
Checking PATCH 4/4: ...
ERROR: suspect code indent for conditional statements (4, 9)
#92: FILE: tests/pnv-xscom-test.c:58:
+    if (chip->chip_type == PNV_CHIP_POWER9) {
+         addr |= ((uint64_t) pcba << 3);

total: 1 errors, 0 warnings, 147 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 0/4] ppc/pnv: XSCOM fixes and unit tests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

On Mon, Nov 14, 2016 at 10:12:54AM +0100, Cédric Le Goater wrote:
> Hello,
> 
> Here is a little serie adding some fixes for the XSCOM registers of
> the POWER9 cores and a unit test. 
> 
> Changes since v1 :
> 
>  - fixed pnv_xscom_addr() for 32bit host systems
>  - replace hweight_long() by ctpop64()
> 
> Tested on a 32-bit LE system and on 64-bit LE and BE systems.

Applied to ppc-for-2.8.

I fixed a trivial nit in 4/4 - you had a 5 space indent, instead of a
4 space indent in one place.

> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (3):
>   ppc/pnv: add a 'xscom_core_base' field to PnvChipClass
>   ppc/pnv: fix xscom address translation for POWER9
>   tests: add XSCOM tests for the PowerNV machine
> 
> David Gibson (1):
>   ppc/pnv: Fix fatal bug on 32-bit hosts
> 
>  hw/ppc/pnv.c               |  10 +++-
>  hw/ppc/pnv_xscom.c         |   8 +--
>  include/hw/ppc/pnv.h       |   1 +
>  include/hw/ppc/pnv_xscom.h |   5 +-
>  tests/Makefile.include     |   1 +
>  tests/pnv-xscom-test.c     | 140 +++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 156 insertions(+), 9 deletions(-)
>  create mode 100644 tests/pnv-xscom-test.c
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] ppc/pnv: XSCOM fixes and unit tests

[Cédric Le Goater]

On 11/15/2016 12:11 AM, David Gibson wrote:
> On Mon, Nov 14, 2016 at 10:12:54AM +0100, Cédric Le Goater wrote:
>> Hello,
>>
>> Here is a little serie adding some fixes for the XSCOM registers of
>> the POWER9 cores and a unit test. 
>>
>> Changes since v1 :
>>
>>  - fixed pnv_xscom_addr() for 32bit host systems
>>  - replace hweight_long() by ctpop64()
>>
>> Tested on a 32-bit LE system and on 64-bit LE and BE systems.
> 
> Applied to ppc-for-2.8.
> 
> I fixed a trivial nit in 4/4 - you had a 5 space indent, instead of a
> 4 space indent in one place.

yes. I need to automate the checkpatch step in some ways when I send 
patches.

Thanks for doing that.

C.