Re: [PATCH v3 08/10] pnv_phb4.c: check stack->phb not NULL in phb4_update_regions()

[Cédric Le Goater]

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);