Re: [Qemu-ppc] [PATCH v2 3/7] ppc/pnv: Add XSCOM infrastructure

[Cédric Le Goater]

On 09/06/2016 02:48 AM, David Gibson wrote:
> On Mon, Sep 05, 2016 at 05:11:53PM +1000, Benjamin Herrenschmidt wrote:
>> On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
>>>> +static XScomDevice *xscom_find_target(XScomState *s, uint32_t
>>> pcb_addr,
>>>> +                                      uint32_t *range)
>>>> +{
>>>> +    BusChild *bc;
>>>> +
>>>> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
>>>> +        DeviceState *qd = bc->child;
>>>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
>>>> +        unsigned int i;
>>>> +
>>>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
>>>> +            if (xd->ranges[i].addr <= pcb_addr &&
>>>> +                (xd->ranges[i].addr + xd->ranges[i].size) >
>>> pcb_addr) {
>>>> +                *range = i;
>>>> +                return xd;
>>>> +            }
>>>> +        }
>>>> +    }
>>>
>>> Hmm.. you could set up a SCOM local address space using the
>>> infrastructure in memory.c, rather than doing your own dispatch.
>>
>> There are pros and cons to this approach. The memory.c stuff comes with
>> quite a lot of baggage, not all of it very shinny to be honest ;-) I
>> still *hate* how it forces upon us a whole 128-bit integer arithmetic
>> library just so that it can represent 1_0000_0000_0000_0000 ... 
> 
> Ugh, yeah.  I tried to argue against this when it first came in, but
> was overruled.
> 
>> It would be make more sense to use inclusive start/end instead and
>> stick to 64-bits.
>>
>> That being said, we could do that. We'd have to shift the XSCOM
>> addresses left by 3 since each address is an 8 bytes reigster and
>> forbid non-8-bytes accesses.
> 
> Ok.  I'm not particularly fussed either way.


The change does seem too invasive. I can give it a try in next
version.

When a memory region is triggered, the impacted device will have
to convert the address with xscom_to_pcb_addr(). There is some 
dependency on the chip model because the translation is different. 
That would be an extra op in the xscom device model I guess. 

Also, the main purpose of the XscomBus is to loop on the devices 
to populate the device tree. I am wondering if we could just use 
a simple list under the chip for that purpose. 

Thanks,

C.