Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure

[Cédric Le Goater]

On 09/23/2016 04:46 AM, David Gibson wrote:
> On Thu, Sep 22, 2016 at 10:25:59AM +0200, Cédric Le Goater wrote:
>>>> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectClass 
>>>> *klass, void *data)
>>>>      k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */
>>>>      k->cores_mask = POWER9_CORE_MASK;
>>>>      k->core_pir = pnv_chip_core_pir_p9;
>>>> +    k->xscom_addr = pnv_chip_xscom_addr_p9;
>>>> +    k->xscom_pcba = pnv_chip_xscom_pcba_p9;
>>>
>>> So if you do as BenH (and I) suggested and have the "scom address
>>> space" actually be addressed by (pcba << 3), I think you can probably
>>> avoid these.  
>>
>> I will look at that option again. 
>>
>> I was trying to untangle a few things at the same time. I have better
>> view of the problem to solve now. The bus is gone, that's was one 
>> thing. How we map these xscom regions is the next. 
>>
>> Ben suggested to add some P7/P8 mangling before the dispatch in 
>> the &address_space_xscom. This should make things cleaner. I had 
>> not thought of doing that and this is why I introduced these helpers :
>>
>> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr)
>> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba)
>>
>> which I don't really like ...
>>
>> but we must make sure that we can do the mapping of the xscom 
>> subregions in the &address_space_xscom using (pcba << 3)
>>
>>
>>> Instead you can handle it in the chip or ADU realize function by either:
>>>
>>>     P8: * map one big subregion for the ADU into &address_space_memory
>>>         * have the handler for that subregion do the address mangling,
>>>           then redispatch into the xscom address space
>>>
>>>     P9: * Map the appropriate chunk of the xscom address space
>>>           directly into address_space_memory
>>
>> Yes that was my feeling for a better solution but Ben chimed in with the 
>> HMER topic. I need to look at that.
> 
> Right.  Doesn't change the basic concept though - it just means you
> need (slightly different) redispatchers for both P8 and P9.

In fact they are the same, you only need an "addr to pcba" handler at the
chip class level : 

static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
{
        PnvChip *chip = opaque;
        uint32_t pcba = PNV_CHIP_GET_CLASS(chip)->xscom_pcba(addr);
        uint64_t val = 0;
        MemTxResult result;

        ...

        val = address_space_ldq(&chip->xscom_as, pcba << 3,
                                MEMTXATTRS_UNSPECIFIED, &result);
        if (result != MEMTX_OK) {

  

And so, the result is pretty clean. I killed the proxy object and merged 
the regions in the chip but I have kept the pnv_xscom.c file because the 
code related to xscom is rather large : ~250 lines. 

The objects declaring a xscom region need to do some register shifting but 
this is usual in mmio regions.

You will see in v4.

>>>> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, uint64_t 
>>>> *val)
>>>> +{
>>>> +    uint32_t success;
>>>> +    uint8_t data[8];
>>>> +
>>>> +    success = !address_space_rw(&xscom->xscom_as, addr, 
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> +                                data, 8, false);
>>>> +    *val = (((uint64_t) data[0]) << 56 |
>>>> +            ((uint64_t) data[1]) << 48 |
>>>> +            ((uint64_t) data[2]) << 40 |
>>>> +            ((uint64_t) data[3]) << 32 |
>>>> +            ((uint64_t) data[4]) << 24 |
>>>> +            ((uint64_t) data[5]) << 16 |
>>>> +            ((uint64_t) data[6]) << 8  |
>>>> +            ((uint64_t) data[7]));
>>>
>>> AFAICT this is basically assuming data is always encoded BE.  With the
>>> right choice of endian flags on the individual SCOM device
>>> registrations with the scom address space, I think you should be able
>>> to avoid this mangling.
>>
>> yes. I should but curiously I had to do this, and this works the same on
>> an intel host or a ppc64 host.
> 
> Hmm.. I suspect what you actually need is NATIVE_ENDIAN on the
> individual SCOM devices, with BIG_ENDIAN on the redispatcher region.

we should be using address_space_ldq and address_space_stq.

>>>> +
>>>> +    success = !address_space_rw(&xscom->xscom_as, addr, 
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> +                           data, 8, true);
>>>> +    return success;
>>>> +}
>>>> +
>>>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
>>>> +{
>>>> +    PnvXScom *s = opaque;
>>>> +    uint32_t pcba = s->chip_class->xscom_pcba(addr);
>>>> +    uint64_t val = 0;
>>>> +
>>>> +    /* Handle some SCOMs here before dispatch */
>>>> +    switch (pcba) {
>>>> +    case 0xf000f:
>>>> +        val = s->chip_class->chip_cfam_id;
>>>> +        break;
>>>> +    case 0x1010c00:     /* PIBAM FIR */
>>>> +    case 0x1010c03:     /* PIBAM FIR MASK */
>>>> +    case 0x2020007:     /* ADU stuff */
>>>> +    case 0x2020009:     /* ADU stuff */
>>>> +    case 0x202000f:     /* ADU stuff */
>>>> +        val = 0;
>>>> +        break;
>>>> +    case 0x2013f00:     /* PBA stuff */
>>>> +    case 0x2013f01:     /* PBA stuff */
>>>> +    case 0x2013f02:     /* PBA stuff */
>>>> +    case 0x2013f03:     /* PBA stuff */
>>>> +    case 0x2013f04:     /* PBA stuff */
>>>> +    case 0x2013f05:     /* PBA stuff */
>>>> +    case 0x2013f06:     /* PBA stuff */
>>>> +    case 0x2013f07:     /* PBA stuff */
>>>> +        val = 0;
>>>> +        break;
>>>
>>> It'd be theoretically nicer to actually register regions for these
>>> special case addresses, but handling it here is a reasonable hack to
>>> get things working quickly for the time being.
>>
>> I will make a default region on the whole xscomm address space to catch 
>> these.
> 
> Ok.

Well, it does not bring much and we loose the ability to catch errors. 
I will leave it that way.

Thanks,

C.