| Message ID | 20220110143346.455901-9-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | user creatable pnv-phb4 devices | expand |
On 1/10/22 15:33, Daniel Henrique Barboza wrote: > The last step before enabling user creatable pnv-phb4 devices still has > to do with specific XSCOM initialization code in pnv_phb4_stk_realize(). > > 'stack->nest_regs_mr' is being init regardless of the existence of > 'stack->phb', which is verified only when trying to realize the phb. > Its MemoryRegionOps,'pnv_pec_stk_nest_xscom_ops', uses > pnv_pec_stk_nest_xscom_write() as a write callback. When trying to write > the PEC_NEST_STK_BAR_EN reg, pnv_pec_stk_update_map() is called. Inside > this function, pnv_phb4_update_regions() is called twice. This function > uses stack->phb to manipulate memory regions of the phb. > > When enabling user creatable phb4s, a stack that doesn't have an > associated phb (i.e. will have stack->phb = NULL) will cause a SIGINT > during boot in pnv_phb4_update_regions(). To deal with this we have > some options, including: > > - check for stack->phb being not NULL in pnv_phb4_update_regions(); > > - change the order of the XSCOM initialization to avoid initializing > 'stack->nest_regs_mr' if 'stack->phb' is NULL. This can have unintended > side-effects: there are several other regs that are being read/written > in these memory regions, and we would forbid all read/write operations > in these regs because of writes in PEC_NEST_STK_BAR_EN being problematic; > > - move the XSCOM init code to phb4_realize() like the previous patch > did with 'stack->phb_regs_mr'. Besides having the same potential side > effects than the previous alternative, a lot of code would need to be > moved from pnv_phb4_pec.c to pnv_phb4.c because all the memory region > code is static. > > Being the option that is less intrusive and innocus of the alternatives, > this patch keeps the XSCOM initialization as is in > pnv_phb4_stk_realize() and check for 'stack->phb' being NULL in > pnv_phb4_update_regions(). > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb4.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index 152911a285..fc23a96b7f 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -1472,6 +1472,17 @@ void pnv_phb4_update_regions(PnvPhb4PecStack *stack) > { > PnvPHB4 *phb = stack->phb; > > + /* > + * This will happen when there's no phb associated with the stack. > + * pnv_pec_stk_realize() will init the nested xscom address space > + * (stack->nest_regs_mr) that uses pnv_phb4_update_regions(), via > + * pnv_pec_stk_update_map(), which in turn is the write callback of > + * the PEC_NEST_STK_BAR_EN reg in pnv_pec_stk_nest_xscom_write(). > + */ > + if (!stack->phb) { > + return; > + } > + I would assert() Thanks, C. > /* Unmap first always */ > if (memory_region_is_mapped(&phb->mr_regs)) { > memory_region_del_subregion(&stack->phbbar, &phb->mr_regs); >
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index 152911a285..fc23a96b7f 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -1472,6 +1472,17 @@ void pnv_phb4_update_regions(PnvPhb4PecStack *stack) { PnvPHB4 *phb = stack->phb; + /* + * This will happen when there's no phb associated with the stack. + * pnv_pec_stk_realize() will init the nested xscom address space + * (stack->nest_regs_mr) that uses pnv_phb4_update_regions(), via + * pnv_pec_stk_update_map(), which in turn is the write callback of + * the PEC_NEST_STK_BAR_EN reg in pnv_pec_stk_nest_xscom_write(). + */ + if (!stack->phb) { + return; + } + /* Unmap first always */ if (memory_region_is_mapped(&phb->mr_regs)) { memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
The last step before enabling user creatable pnv-phb4 devices still has to do with specific XSCOM initialization code in pnv_phb4_stk_realize(). 'stack->nest_regs_mr' is being init regardless of the existence of 'stack->phb', which is verified only when trying to realize the phb. Its MemoryRegionOps,'pnv_pec_stk_nest_xscom_ops', uses pnv_pec_stk_nest_xscom_write() as a write callback. When trying to write the PEC_NEST_STK_BAR_EN reg, pnv_pec_stk_update_map() is called. Inside this function, pnv_phb4_update_regions() is called twice. This function uses stack->phb to manipulate memory regions of the phb. When enabling user creatable phb4s, a stack that doesn't have an associated phb (i.e. will have stack->phb = NULL) will cause a SIGINT during boot in pnv_phb4_update_regions(). To deal with this we have some options, including: - check for stack->phb being not NULL in pnv_phb4_update_regions(); - change the order of the XSCOM initialization to avoid initializing 'stack->nest_regs_mr' if 'stack->phb' is NULL. This can have unintended side-effects: there are several other regs that are being read/written in these memory regions, and we would forbid all read/write operations in these regs because of writes in PEC_NEST_STK_BAR_EN being problematic; - move the XSCOM init code to phb4_realize() like the previous patch did with 'stack->phb_regs_mr'. Besides having the same potential side effects than the previous alternative, a lot of code would need to be moved from pnv_phb4_pec.c to pnv_phb4.c because all the memory region code is static. Being the option that is less intrusive and innocus of the alternatives, this patch keeps the XSCOM initialization as is in pnv_phb4_stk_realize() and check for 'stack->phb' being NULL in pnv_phb4_update_regions(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb4.c | 11 +++++++++++ 1 file changed, 11 insertions(+)