[PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Nicholas Piggin]

The P10 core xscom memory regions overlap because the size is wrong.
The P10 core+L2 xscom region size is allocated as 0x1000 (with some
unused ranges). "EC" is used as a closer match, as "EX" includes L3
which has a disjoint xscom range that would require a different
region if it were implemented.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/pnv_core.c          | 3 +--
 include/hw/ppc/pnv_xscom.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index b7223bb445..ffbc29cbf4 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -299,9 +299,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
-    /* TODO: check PNV_XSCOM_EX_SIZE for p10 */
     pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
-                          pc, name, PNV_XSCOM_EX_SIZE);
+                          pc, name, PNV10_XSCOM_EC_SIZE);
 
     qemu_register_reset(pnv_core_reset, pc);
     return;
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index f7da9a1dc6..a4c9d95dc5 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -133,7 +133,7 @@ struct PnvXScomInterfaceClass {
 
 #define PNV10_XSCOM_EC_BASE(core) \
     ((uint64_t) PNV10_XSCOM_EQ_BASE(core) | PNV10_XSCOM_EC(core & 0x3))
-#define PNV10_XSCOM_EC_SIZE        0x100000
+#define PNV10_XSCOM_EC_SIZE        0x1000
 
 #define PNV10_XSCOM_PSIHB_BASE     0x3011D00
 #define PNV10_XSCOM_PSIHB_SIZE     0x100
-- 
2.40.1

Re: [PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Joel Stanley]

On Wed, 5 Jul 2023 at 01:27, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> The P10 core xscom memory regions overlap because the size is wrong.
> The P10 core+L2 xscom region size is allocated as 0x1000 (with some
> unused ranges). "EC" is used as a closer match, as "EX" includes L3
> which has a disjoint xscom range that would require a different
> region if it were implemented.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Nice, that looks better:

0000000100000000-00000001000fffff (prio 0, i/o): xscom-quad.0: 0x100000
0000000100108000-000000010010ffff (prio 0, i/o): xscom-core.3: 0x8000
0000000100110000-0000000100117fff (prio 0, i/o): xscom-core.2: 0x8000
0000000100120000-0000000100127fff (prio 0, i/o): xscom-core.1: 0x8000
0000000100140000-0000000100147fff (prio 0, i/o): xscom-core.0: 0x8000
0000000108000000-00000001080fffff (prio 0, i/o): xscom-quad.4: 0x100000
0000000108108000-000000010810ffff (prio 0, i/o): xscom-core.7: 0x8000
0000000108110000-0000000108117fff (prio 0, i/o): xscom-core.6: 0x8000
0000000108120000-0000000108127fff (prio 0, i/o): xscom-core.5: 0x8000
0000000108140000-0000000108147fff (prio 0, i/o): xscom-core.4: 0x8000

Reviewed-by: Joel Stanley <joel@jms.id.au>


> ---
>  hw/ppc/pnv_core.c          | 3 +--
>  include/hw/ppc/pnv_xscom.h | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index b7223bb445..ffbc29cbf4 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -299,9 +299,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>
>      snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
> -    /* TODO: check PNV_XSCOM_EX_SIZE for p10 */
>      pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
> -                          pc, name, PNV_XSCOM_EX_SIZE);
> +                          pc, name, PNV10_XSCOM_EC_SIZE);
>
>      qemu_register_reset(pnv_core_reset, pc);
>      return;
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index f7da9a1dc6..a4c9d95dc5 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -133,7 +133,7 @@ struct PnvXScomInterfaceClass {
>
>  #define PNV10_XSCOM_EC_BASE(core) \
>      ((uint64_t) PNV10_XSCOM_EQ_BASE(core) | PNV10_XSCOM_EC(core & 0x3))
> -#define PNV10_XSCOM_EC_SIZE        0x100000
> +#define PNV10_XSCOM_EC_SIZE        0x1000
>
>  #define PNV10_XSCOM_PSIHB_BASE     0x3011D00
>  #define PNV10_XSCOM_PSIHB_SIZE     0x100
> --
> 2.40.1
>

Re: [PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Cédric Le Goater]

On 7/5/23 04:05, Joel Stanley wrote:
> On Wed, 5 Jul 2023 at 01:27, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> The P10 core xscom memory regions overlap because the size is wrong.
>> The P10 core+L2 xscom region size is allocated as 0x1000 (with some
>> unused ranges). "EC" is used as a closer match, as "EX" includes L3
>> which has a disjoint xscom range that would require a different
>> region if it were implemented.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Nice, that looks better:
> 
> 0000000100000000-00000001000fffff (prio 0, i/o): xscom-quad.0: 0x100000
> 0000000100108000-000000010010ffff (prio 0, i/o): xscom-core.3: 0x8000
> 0000000100110000-0000000100117fff (prio 0, i/o): xscom-core.2: 0x8000
> 0000000100120000-0000000100127fff (prio 0, i/o): xscom-core.1: 0x8000
> 0000000100140000-0000000100147fff (prio 0, i/o): xscom-core.0: 0x8000
> 0000000108000000-00000001080fffff (prio 0, i/o): xscom-quad.4: 0x100000
> 0000000108108000-000000010810ffff (prio 0, i/o): xscom-core.7: 0x8000
> 0000000108110000-0000000108117fff (prio 0, i/o): xscom-core.6: 0x8000
> 0000000108120000-0000000108127fff (prio 0, i/o): xscom-core.5: 0x8000
> 0000000108140000-0000000108147fff (prio 0, i/o): xscom-core.4: 0x8000
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>

It'd interesting to add some dummy SLW handlers to get rid of the
XSCOM errors at boot and shutdown on P10 :

[ 4824.393446266,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
[ 4824.393588777,5] Unable to log error
[ 4824.393650582,3] XSCOM: Write failed, ret =  -6
[ 4824.394124623,3] Could not set special wakeup on 0:0: Unable to write QME_SPWU_HYP.
[ 4824.394368459,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
[ 4824.394382007,5] Unable to log error
[ 4824.394384603,3] XSCOM: Write failed, ret =  -6

Thanks,

C.

> 
> 
>> ---
>>   hw/ppc/pnv_core.c          | 3 +--
>>   include/hw/ppc/pnv_xscom.h | 2 +-
>>   2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index b7223bb445..ffbc29cbf4 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -299,9 +299,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>       }
>>
>>       snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
>> -    /* TODO: check PNV_XSCOM_EX_SIZE for p10 */
>>       pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
>> -                          pc, name, PNV_XSCOM_EX_SIZE);
>> +                          pc, name, PNV10_XSCOM_EC_SIZE);
>>
>>       qemu_register_reset(pnv_core_reset, pc);
>>       return;
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> index f7da9a1dc6..a4c9d95dc5 100644
>> --- a/include/hw/ppc/pnv_xscom.h
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -133,7 +133,7 @@ struct PnvXScomInterfaceClass {
>>
>>   #define PNV10_XSCOM_EC_BASE(core) \
>>       ((uint64_t) PNV10_XSCOM_EQ_BASE(core) | PNV10_XSCOM_EC(core & 0x3))
>> -#define PNV10_XSCOM_EC_SIZE        0x100000
>> +#define PNV10_XSCOM_EC_SIZE        0x1000
>>
>>   #define PNV10_XSCOM_PSIHB_BASE     0x3011D00
>>   #define PNV10_XSCOM_PSIHB_SIZE     0x100
>> --
>> 2.40.1
>>

Re: [PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Daniel Henrique Barboza]

This patch breaks make check-qtest:


$ make -j -C build && make -C build check-qtest
(...)
16/44 qemu:qtest+qtest-ppc64 / qtest-ppc64/pnv-xscom-test                 ERROR           0.89s   killed by signal 6 SIGABRT
>>> G_TEST_DBUS_DAEMON=/home/danielhb/powerpc/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-ppc64 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon MALLOC_PERTURB_=231 /home/danielhb/powerpc/qemu/build/tests/qtest/pnv-xscom-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
**
ERROR:../tests/qtest/pnv-xscom-test.c:115:test_xscom_core: assertion failed (dts0 == 0x26f024f023f0000ull): (0x00000000 == 0x26f024f023f0000)

(test program exited with status code -6)
――――――――――――――――――――――――――――――――――――――――――――――――

I believe you'll need to take a look at tests/qtest/pnv-xscom-test.c as well.


Thanks,

Daniel


On 7/4/23 22:27, Nicholas Piggin wrote:
> The P10 core xscom memory regions overlap because the size is wrong.
> The P10 core+L2 xscom region size is allocated as 0x1000 (with some
> unused ranges). "EC" is used as a closer match, as "EX" includes L3
> which has a disjoint xscom range that would require a different
> region if it were implemented.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/pnv_core.c          | 3 +--
>   include/hw/ppc/pnv_xscom.h | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index b7223bb445..ffbc29cbf4 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -299,9 +299,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>       }
>   
>       snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
> -    /* TODO: check PNV_XSCOM_EX_SIZE for p10 */
>       pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
> -                          pc, name, PNV_XSCOM_EX_SIZE);
> +                          pc, name, PNV10_XSCOM_EC_SIZE);
>   
>       qemu_register_reset(pnv_core_reset, pc);
>       return;
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index f7da9a1dc6..a4c9d95dc5 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -133,7 +133,7 @@ struct PnvXScomInterfaceClass {
>   
>   #define PNV10_XSCOM_EC_BASE(core) \
>       ((uint64_t) PNV10_XSCOM_EQ_BASE(core) | PNV10_XSCOM_EC(core & 0x3))
> -#define PNV10_XSCOM_EC_SIZE        0x100000
> +#define PNV10_XSCOM_EC_SIZE        0x1000
>   
>   #define PNV10_XSCOM_PSIHB_BASE     0x3011D00
>   #define PNV10_XSCOM_PSIHB_SIZE     0x100

Re: [PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Joel Stanley]

On Wed, 5 Jul 2023 at 10:02, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 7/5/23 04:05, Joel Stanley wrote:
> > On Wed, 5 Jul 2023 at 01:27, Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> The P10 core xscom memory regions overlap because the size is wrong.
> >> The P10 core+L2 xscom region size is allocated as 0x1000 (with some
> >> unused ranges). "EC" is used as a closer match, as "EX" includes L3
> >> which has a disjoint xscom range that would require a different
> >> region if it were implemented.
> >>
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >
> > Nice, that looks better:
> >
> > 0000000100000000-00000001000fffff (prio 0, i/o): xscom-quad.0: 0x100000
> > 0000000100108000-000000010010ffff (prio 0, i/o): xscom-core.3: 0x8000
> > 0000000100110000-0000000100117fff (prio 0, i/o): xscom-core.2: 0x8000
> > 0000000100120000-0000000100127fff (prio 0, i/o): xscom-core.1: 0x8000
> > 0000000100140000-0000000100147fff (prio 0, i/o): xscom-core.0: 0x8000
> > 0000000108000000-00000001080fffff (prio 0, i/o): xscom-quad.4: 0x100000
> > 0000000108108000-000000010810ffff (prio 0, i/o): xscom-core.7: 0x8000
> > 0000000108110000-0000000108117fff (prio 0, i/o): xscom-core.6: 0x8000
> > 0000000108120000-0000000108127fff (prio 0, i/o): xscom-core.5: 0x8000
> > 0000000108140000-0000000108147fff (prio 0, i/o): xscom-core.4: 0x8000
> >
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> It'd interesting to add some dummy SLW handlers to get rid of the
> XSCOM errors at boot and shutdown on P10 :
>
> [ 4824.393446266,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
> [ 4824.393588777,5] Unable to log error
> [ 4824.393650582,3] XSCOM: Write failed, ret =  -6
> [ 4824.394124623,3] Could not set special wakeup on 0:0: Unable to write QME_SPWU_HYP.
> [ 4824.394368459,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
> [ 4824.394382007,5] Unable to log error
> [ 4824.394384603,3] XSCOM: Write failed, ret =  -6

Yes. I was looking at this yesterday. We need to figure out how to do
the xscom addressing for the QME. It sets (different) bits in order to
address a given core.

For a -smp 4 machine, the P10_QME_SPWU_HYP read comes in on these addresses:

    case 0x200e883c:
    case 0x200e483c:
    case 0x200e283c:
    case 0x200e183c:

ie, the fourth nibble selects the core.

For a -smp 8 machine, the address now has bit 24 set to select the
second quad, so we need to cover these addresses:

    case 0x210e883c:
    case 0x210e483c:
    case 0x210e283c:
    case 0x210e183c:

I am thinking about how to map this into an address range that a model
can claim.

Cheers,

Joel

PS. For reference, this is sufficient to silence xscom errors with
skiboot and -M powernv10 -smp4. A different set of hacks is required
for p9.

--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -106,6 +106,26 @@ static uint64_t xscom_read_default(PnvChip *chip,
uint32_t pcba)
     case 0x401082a:
     case 0x4010828:
         return 0;
+
+    /* P10_QME_SPWU_HYP */
+    case 0x200e883c:
+    case 0x200e483c:
+    case 0x200e283c:
+    case 0x200e183c:
+        return 0;
+
+    /* P10_QME_SSH_HYP */
+    case 0x200e882c:
+    case 0x200e482c:
+    case 0x200e282c:
+    case 0x200e182c:
+        return 0;
+
+    /* XPEC_P10_PCI_CPLT_CONF1 */
+    case 0x08000009:
+    case 0x09000009:
+        return 0;
+
     default:
         return -1;
     }
@@ -152,6 +172,13 @@ static bool xscom_write_default(PnvChip *chip,
uint32_t pcba, uint64_t val)
     case PRD_P8_IPOLL_REG_STATUS:
     case PRD_P9_IPOLL_REG_MASK:
     case PRD_P9_IPOLL_REG_STATUS:
+
+    /* P10_QME_SPWU_HYP */
+    case 0x200e883c:
+    case 0x200e483c:
+    case 0x200e283c:
+    case 0x200e183c:
+
         return true;
     default:
         return false;

Re: [PATCH] ppc/pnv: Set P10 core xscom region size to match hardware

[Cédric Le Goater]

On 7/6/23 04:33, Joel Stanley wrote:
> On Wed, 5 Jul 2023 at 10:02, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 7/5/23 04:05, Joel Stanley wrote:
>>> On Wed, 5 Jul 2023 at 01:27, Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>
>>>> The P10 core xscom memory regions overlap because the size is wrong.
>>>> The P10 core+L2 xscom region size is allocated as 0x1000 (with some
>>>> unused ranges). "EC" is used as a closer match, as "EX" includes L3
>>>> which has a disjoint xscom range that would require a different
>>>> region if it were implemented.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>
>>> Nice, that looks better:
>>>
>>> 0000000100000000-00000001000fffff (prio 0, i/o): xscom-quad.0: 0x100000
>>> 0000000100108000-000000010010ffff (prio 0, i/o): xscom-core.3: 0x8000
>>> 0000000100110000-0000000100117fff (prio 0, i/o): xscom-core.2: 0x8000
>>> 0000000100120000-0000000100127fff (prio 0, i/o): xscom-core.1: 0x8000
>>> 0000000100140000-0000000100147fff (prio 0, i/o): xscom-core.0: 0x8000
>>> 0000000108000000-00000001080fffff (prio 0, i/o): xscom-quad.4: 0x100000
>>> 0000000108108000-000000010810ffff (prio 0, i/o): xscom-core.7: 0x8000
>>> 0000000108110000-0000000108117fff (prio 0, i/o): xscom-core.6: 0x8000
>>> 0000000108120000-0000000108127fff (prio 0, i/o): xscom-core.5: 0x8000
>>> 0000000108140000-0000000108147fff (prio 0, i/o): xscom-core.4: 0x8000
>>>
>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>
>> It'd interesting to add some dummy SLW handlers to get rid of the
>> XSCOM errors at boot and shutdown on P10 :
>>
>> [ 4824.393446266,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
>> [ 4824.393588777,5] Unable to log error
>> [ 4824.393650582,3] XSCOM: Write failed, ret =  -6
>> [ 4824.394124623,3] Could not set special wakeup on 0:0: Unable to write QME_SPWU_HYP.
>> [ 4824.394368459,3] XSCOM: write error gcid=0x0 pcb_addr=0x200e883c stat=0x0
>> [ 4824.394382007,5] Unable to log error
>> [ 4824.394384603,3] XSCOM: Write failed, ret =  -6
> 
> Yes. I was looking at this yesterday. We need to figure out how to do
> the xscom addressing for the QME. It sets (different) bits in order to
> address a given core.>
>
> For a -smp 4 machine, the P10_QME_SPWU_HYP read comes in on these addresses:
> 
>      case 0x200e883c:
>      case 0x200e483c:
>      case 0x200e283c:
>      case 0x200e183c:
> 
> ie, the fourth nibble selects the core.
> 
> For a -smp 8 machine, the address now has bit 24 set to select the
> second quad, so we need to cover these addresses:
> 
>      case 0x210e883c:
>      case 0x210e483c:
>      case 0x210e283c:
>      case 0x210e183c:
> 
> I am thinking about how to map this into an address range that a model
> can claim.
  
OPAL should have the magic macros for XSCOM addresses. I included a few
in QEMU.

> Cheers,
> 
> Joel
> 
> PS. For reference, this is sufficient to silence xscom errors with
> skiboot and -M powernv10 -smp4. A different set of hacks is required
> for p9.
>

Thanks,

C.


> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -106,6 +106,26 @@ static uint64_t xscom_read_default(PnvChip *chip,
> uint32_t pcba)
>       case 0x401082a:
>       case 0x4010828:
>           return 0;
> +
> +    /* P10_QME_SPWU_HYP */
> +    case 0x200e883c:
> +    case 0x200e483c:
> +    case 0x200e283c:
> +    case 0x200e183c:
> +        return 0;
> +
> +    /* P10_QME_SSH_HYP */
> +    case 0x200e882c:
> +    case 0x200e482c:
> +    case 0x200e282c:
> +    case 0x200e182c:
> +        return 0;
> +
> +    /* XPEC_P10_PCI_CPLT_CONF1 */
> +    case 0x08000009:
> +    case 0x09000009:
> +        return 0;
> +
>       default:
>           return -1;
>       }
> @@ -152,6 +172,13 @@ static bool xscom_write_default(PnvChip *chip,
> uint32_t pcba, uint64_t val)
>       case PRD_P8_IPOLL_REG_STATUS:
>       case PRD_P9_IPOLL_REG_MASK:
>       case PRD_P9_IPOLL_REG_STATUS:
> +
> +    /* P10_QME_SPWU_HYP */
> +    case 0x200e883c:
> +    case 0x200e483c:
> +    case 0x200e283c:
> +    case 0x200e183c:
> +
>           return true;
>       default:
>           return false;