Re: [PATCH v3 07/10] ppc/pnv: move PHB4 related XSCOM init to phb4_realize()

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 1/10/22 12:57, Cédric Le Goater wrote:
> On 1/10/22 15:33, Daniel Henrique Barboza wrote:
>> Before enabling pnv-phb4 user creatable devices we need to handle PHB4
>> specific code in pnv_pec_stk_realize().
>>
>> The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
>> 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
>> and add_subregion() to phb4_realize() time is a natural thing to do and
>> it'll spare us from checking 'phb->stack != NULL' in stk_realize() when
>> user creatable pnv-phb4s are implemented.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c     | 27 +++++++++++++++++++++++++++
>>   hw/pci-host/pnv_phb4_pec.c | 10 ----------
>>   2 files changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 1a7395772f..152911a285 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
>>                                &error_abort);
>>   }
>> +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb)
>> +{
>> +    PnvPhb4PecState *pec;
>> +    PnvPhb4PecClass *pecc;
>> +    uint32_t pec_pci_base;
>> +    char name[64];
>> +
>> +    assert(phb->stack);
>> +
>> +    pec = phb->stack->pec;
>> +    pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> +    pec_pci_base = pecc->xscom_pci_base(pec);
>> +
>> +    /* PHB pass-through */
>> +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
>> +             pec->chip_id, pec->index, phb->stack->stack_no);
>> +    pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb),
>> +                          &pnv_phb4_xscom_ops, phb, name, 0x40);
>> +
>> +    pnv_xscom_add_subregion(pec->chip,
>> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> +                            0x40 * phb->stack->stack_no,
>> +                            &phb->stack->phb_regs_mr);
>> +}
>> +
>>   static void pnv_phb4_instance_init(Object *obj)
>>   {
>>       PnvPHB4 *phb = PNV_PHB4(obj);
>> @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>       memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb,
>>                             name, 0x2000);
>> +    pnv_phb4_init_xscom_passthrough(phb);
>> +
>>       /*
>>        * PHB4 doesn't support IO space. However, qemu gets very upset if
>>        * we don't have an IO region to anchor IO BARs onto so we just
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 042dc0b775..5e02a51f04 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -571,12 +571,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>                             &pnv_pec_stk_pci_xscom_ops, stack, name,
>>                             PHB4_PEC_PCI_STK_REGS_COUNT);
>> -    /* PHB pass-through */
>> -    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
>> -             pec->chip_id, pec->index, stack->stack_no);
>> -    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb),
>> -                          &pnv_phb4_xscom_ops, stack->phb, name, 0x40);
>> -
>>       if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>>           return;
>>       }
>> @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>       pnv_xscom_add_subregion(chip,
>>                               pec_pci_base + 0x40 * (stack->stack_no + 1),
>>                               &stack->pci_regs_mr);
>> -    pnv_xscom_add_subregion(chip,
>> -                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> -                            0x40 * stack->stack_no,
>> -                            &stack->phb_regs_mr);
>>   }
> 
> 
> Can't we simply move the XSCOM init and mapping under phb4 realize routine ?

There's a lot of code to be moved when doing that but in theory we can. It would
simplify some things (e.g. we wouldn't need to check stack->phb != NULL in
phb4_update_maps() like we need to do in the next patch).

I also got cold feet about the repercussions of creating a stack without any XSCOM
mapped on it, but perhaps this is the right time to see if that's possible and, if not,
try to understand why.


Thanks,


Daniel



> 
> Thanks,
> 
> C.
> 
> 
>>   static Property pnv_pec_stk_properties[] = {
>>
>