Re: [Qemu-ppc] [PATCH] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge

[Cédric Le Goater]

On 06/27/2018 09:28 AM, David Gibson wrote:
> On Wed, Jun 27, 2018 at 11:38:17AM +1000, Benjamin Herrenschmidt wrote:
>> On Wed, 2018-06-27 at 03:35 +0300, Michael S. Tsirkin wrote:
>>>
>>>> +
>>>> +/* Extract field fname from val */
>>>> +#define GETFIELD(fname, val)                    \
>>>> +        (((val) & fname##_MASK) >> fname##_LSH)
>>>> +
>>>> +/* Set field fname of oval to fval
>>>> + * NOTE: oval isn't modified, the combined result is returned
>>>> + */
>>>> +#define SETFIELD(fname, oval, fval)                     \
>>>> +        (((oval) & ~fname##_MASK) | \
>>>> +         ((((typeof(oval))(fval)) << fname##_LSH) & fname##_MASK))
>>>> +
>>>
>>> Pls don't make up macros like these. We can't have each device do it.
>>
>> So what ? We move the macros in a generic place ? These are MUCH better
>> than open-coding the masks & shifts and much less error prone.
> 
> There are already deposit32 and deposit64() functions which I think
> would cover this.

OK. I understand but we can't include target/ppc/cpu.h in this file
either and we want to use these specific macros to keep the register 
definition file similar to the one in skiboot.

Is their a common area for such needs ? 

>>
>>>> @@ -1031,6 +1110,7 @@ static Property pnv_chip_properties[] = {
>>>>      DEFINE_PROP_UINT64("ram-size", PnvChip, ram_size, 0),
>>>>      DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1),
>>>>      DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0),
>>>> +    DEFINE_PROP_UINT32("num-phbs", PnvChip, num_phbs, 1),
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>
>>> How about instanciating each extra phb using -device?
>>> Seems better than teaching everyone about platform-specific
>>> options.
>>
>> It's about which PHBs are enabled, not which are instanciated, if I
>> understand Cedric changes ...
>>
>> This aims are implementing the POWER8 chip correctly, it has a fixed
>> number of PHBs per-chip at very specific XSCOM addresses, that the
>> firwmare knows about.
> 
> Yeah, this is a recurring design conflict, which I'm not sure how to
> address.  Usually instantiating things with -device works better, but
> when do we do when that allows more flexibility with hardware
> arrangement than is actually possible in the hardware.
> 

So the "IBM PHB3 PCIE Root Port" is already user createable.

I can take a look at user createable PHB3s. I think this is OK from a model
perspective. The object is rather standalone, it needs the machine for 
the XICS fabric and a couple of ids, phb id and chip id. These can come
from the command line.

We want at least one PHB3 per socket/chip though. 

C.