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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8100 bytes --]

On Tue, Sep 27, 2016 at 07:54:37AM +0200, Cédric Le Goater wrote:
> On 09/27/2016 04:35 AM, David Gibson wrote:
> > On Mon, Sep 26, 2016 at 06:11:36PM +0200, Cédric Le Goater wrote:
> >> 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 : 
> > 
> > Ok.  I'd been thinking of using different dispatchers as an
> > alternative to using the chip class translator hook, 
> 
> ah. yes, why not. We could have per-chip dispatchers but they 
> would have a lot in common.

Would they?  Unless you're counting the core register dispatch - and
it sounds like splitting that for P8 vs. P9 would be a good idea
anyway - I don't see that there's much in common besides the address
translation.

Note of course, that you can add a helper function that both
dispatchers can use if it's useful.

> However, I think we can get rid of 
> the xscom_pcba' handlers, they should not be needed any where 
> else than in the XSCOM dispatchers. 
> 
> > but I guess if you have the decoding of those "core" registers 
> > here as well, then that doesn't make so much sense.
> 
> yes and there is also the handling of the XSCOM failures.

Hm, ok.

> I can add some prologue handler to cover those "core" registers
> but adding a MemoryRegion, ops, init and mapping would be a lot 
> of churn just to return 0.
> 
> Thanks,
> 
> C. 
> 
> 
> >> 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. 
> > 
> > Sure, makes sense.
> > 
> >> The objects declaring a xscom region need to do some register shifting but 
> >> this is usual in mmio regions.
> >>
> >> You will see in v4.
> > 
> > Ok.
> > 
> >>>>>> +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.
> > 
> > Ok.
> > 
> >>>>>> +
> >>>>>> +    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. 
> >>
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]