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

2022-01-12 Thread 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 
---



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 



  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 & 0x8000ull;
+stack->pci_regs[reg] = val & 0x8000ull;
  /* 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

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

2022-01-11 Thread 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 
---
 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 & 0x8000ull;
+stack->pci_regs[reg] = val & 0x8000ull;
 /* 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