Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Frederic Barrat <fbarrat@linux.ibm.com>]

On 02/03/2023 23:21, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
> 
> On 2/3/23 17:37, Frederic Barrat wrote:
>> Add an array on the PEC object to keep track of the PHBs which are
>> instantiated. The array can be sparsely populated when using
>> user-created PHBs. It will be useful for the next patch to only export
>> instantiated PHBs in the device tree.
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
>>   hw/ppc/pnv.c                   |  1 +
>>   include/hw/pci-host/pnv_phb4.h |  2 ++
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 43267a428f..97c06bb0a0 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
> 
>> +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>> +                                           int stack_no,
>> +                                           Error **errp)
>>   {
>>       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
>>       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
>> @@ -128,8 +128,9 @@ static void 
>> pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
>>                               &error_fatal);
>>       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
>> -        return;
>> +        return NULL;
>>       }
>> +    return phb;
>>   }
> 
> 
>> diff --git a/include/hw/pci-host/pnv_phb4.h 
>> b/include/hw/pci-host/pnv_phb4.h
>> index 28d61b96c7..0b72ef1471 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
>>       /* PHBs */
>>       uint32_t num_phbs;
>> +#define MAX_PHBS_PER_PEC        3
>> +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
>>       PnvChip *chip;
>>   };
> 
>  From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
> a pointer to it), and initialize the PnvPHB instance calling
> object_initialize_child() instead of qdev_new().

Hi Phil,
Daniel beat me to it, but we used to do precisely that but went the
opposite direction (see 0d512c7120a2), because we can now specify from 
the command line what PHB to implement so we don't want to allocate all 
of them.

   Fred


> 
> See for example the recent conversion of OHCISysBusState in commit
> 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").
> 
> Regards,
> 
> Phil.