[PATCH 0/1] ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

[PATCH 0/1] ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

[Daniel Henrique Barboza]

Hi,

This is something that caught my eye when I was looking into the
instances where we need stack properties versus phb4 properties.

I tested this fix and it doesn't seem to impact the boot process
whatsoever. Tracing pnv_pec_stk_pci_xscom_write() shows that the writes
are being done at early boot and then nothing else. There might be a
future bug that we're fixing beforehand with this patch as well.

At the very least the code now makes more sense, at least in my
estimation.

Daniel Henrique Barboza (1):
  ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

 hw/pci-host/pnv_phb4.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.33.1

[PATCH 1/1] ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

[Daniel Henrique Barboza]

pnv_pec_stk_pci_xscom_write() is pnv_pec_stk_pci_xscom_ops write
callback. It writes values into regs in the stack->nest_regs[] array.
The pnv_pec_stk_pci_xscom_read read callback, on the other hand, returns
values of the stack->pci_regs[]. In fact, at this moment, the only use
of stack->pci_regs[] is in pnv_pec_stk_pci_xscom_read(). There's no code
that is written anything in stack->pci_regs[], which is suspicious.

Considering that stack->nest_regs[] is widely used by the nested
MemoryOps pnv_pec_stk_nest_xscom_ops, in both read and write callbacks,
the conclusion is that we're writing the wrong array in
pnv_pec_stk_pci_xscom_write(). This function should write stack->pci_regs[]
instead.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index be29174f13..a7b638831e 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1086,39 +1086,39 @@ static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
 
     switch (reg) {
     case PEC_PCI_STK_PCI_FIR:
-        stack->nest_regs[reg] = val;
+        stack->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_CLR:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR] &= val;
+        stack->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
         break;
     case PEC_PCI_STK_PCI_FIR_SET:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR] |= val;
+        stack->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSK:
-        stack->nest_regs[reg] = val;
+        stack->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSKC:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
+        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
         break;
     case PEC_PCI_STK_PCI_FIR_MSKS:
-        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
+        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
         break;
     case PEC_PCI_STK_PCI_FIR_ACT0:
     case PEC_PCI_STK_PCI_FIR_ACT1:
-        stack->nest_regs[reg] = val;
+        stack->pci_regs[reg] = val;
         break;
     case PEC_PCI_STK_PCI_FIR_WOF:
-        stack->nest_regs[reg] = 0;
+        stack->pci_regs[reg] = 0;
         break;
     case PEC_PCI_STK_ETU_RESET:
-        stack->nest_regs[reg] = val & 0x8000000000000000ull;
+        stack->pci_regs[reg] = val & 0x8000000000000000ull;
         /* TODO: Implement reset */
         break;
     case PEC_PCI_STK_PBAIB_ERR_REPORT:
         break;
     case PEC_PCI_STK_PBAIB_TX_CMD_CRED:
     case PEC_PCI_STK_PBAIB_TX_DAT_CRED:
-        stack->nest_regs[reg] = val;
+        stack->pci_regs[reg] = val;
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx
-- 
2.33.1

Re: [PATCH 1/1] ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

[Frederic Barrat]

On 11/01/2022 21:01, Daniel Henrique Barboza wrote:
> pnv_pec_stk_pci_xscom_write() is pnv_pec_stk_pci_xscom_ops write
> callback. It writes values into regs in the stack->nest_regs[] array.
> The pnv_pec_stk_pci_xscom_read read callback, on the other hand, returns
> values of the stack->pci_regs[]. In fact, at this moment, the only use
> of stack->pci_regs[] is in pnv_pec_stk_pci_xscom_read(). There's no code
> that is written anything in stack->pci_regs[], which is suspicious.
> 
> Considering that stack->nest_regs[] is widely used by the nested
> MemoryOps pnv_pec_stk_nest_xscom_ops, in both read and write callbacks,
> the conclusion is that we're writing the wrong array in
> pnv_pec_stk_pci_xscom_write(). This function should write stack->pci_regs[]
> instead.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


I guess it shows how much those registers are used with our model :-) 
They are mostly FIR registers...
Looks good to me.
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>


>   hw/pci-host/pnv_phb4.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index be29174f13..a7b638831e 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1086,39 +1086,39 @@ static void pnv_pec_stk_pci_xscom_write(void *opaque, hwaddr addr,
>   
>       switch (reg) {
>       case PEC_PCI_STK_PCI_FIR:
> -        stack->nest_regs[reg] = val;
> +        stack->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_CLR:
> -        stack->nest_regs[PEC_PCI_STK_PCI_FIR] &= val;
> +        stack->pci_regs[PEC_PCI_STK_PCI_FIR] &= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_SET:
> -        stack->nest_regs[PEC_PCI_STK_PCI_FIR] |= val;
> +        stack->pci_regs[PEC_PCI_STK_PCI_FIR] |= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSK:
> -        stack->nest_regs[reg] = val;
> +        stack->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSKC:
> -        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
> +        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] &= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_MSKS:
> -        stack->nest_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
> +        stack->pci_regs[PEC_PCI_STK_PCI_FIR_MSK] |= val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_ACT0:
>       case PEC_PCI_STK_PCI_FIR_ACT1:
> -        stack->nest_regs[reg] = val;
> +        stack->pci_regs[reg] = val;
>           break;
>       case PEC_PCI_STK_PCI_FIR_WOF:
> -        stack->nest_regs[reg] = 0;
> +        stack->pci_regs[reg] = 0;
>           break;
>       case PEC_PCI_STK_ETU_RESET:
> -        stack->nest_regs[reg] = val & 0x8000000000000000ull;
> +        stack->pci_regs[reg] = val & 0x8000000000000000ull;
>           /* TODO: Implement reset */
>           break;
>       case PEC_PCI_STK_PBAIB_ERR_REPORT:
>           break;
>       case PEC_PCI_STK_PBAIB_TX_CMD_CRED:
>       case PEC_PCI_STK_PBAIB_TX_DAT_CRED:
> -        stack->nest_regs[reg] = val;
> +        stack->pci_regs[reg] = val;
>           break;
>       default:
>           qemu_log_mask(LOG_UNIMP, "phb4_pec_stk: pci_xscom_write 0x%"HWADDR_PRIx

Re: [PATCH 0/1] ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()

[Cédric Le Goater]

On 1/11/22 21:01, Daniel Henrique Barboza wrote:
> Hi,
> 
> This is something that caught my eye when I was looking into the
> instances where we need stack properties versus phb4 properties.
> 
> I tested this fix and it doesn't seem to impact the boot process
> whatsoever. Tracing pnv_pec_stk_pci_xscom_write() shows that the writes
> are being done at early boot and then nothing else. There might be a
> future bug that we're fixing beforehand with this patch as well.
> 
> At the very least the code now makes more sense, at least in my
> estimation.
> 
> Daniel Henrique Barboza (1):
>    ppc/pnv: use stack->pci_regs[] in pnv_pec_stk_pci_xscom_write()
> 
>   hw/pci-host/pnv_phb4.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 


Applied to ppc7.0.

Thanks,

C.