Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
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 = 0x100d10498000ull; /* 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 |
> >> +
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Tue, 2016-09-27 at 11:10 +0200, Cédric Le Goater wrote: > > > > > > > > +PowerPCCPU *cpu = POWERPC_CPU(cs); > > > +CPUPPCState *env = &cpu->env; > > > + > > > +cpu_synchronize_state(cs); > > > +env->spr[SPR_HMER] |= hmer_bits; > > > + > > > +/* XXX Need a CPU helper to set HMER, also handle gneeration > > > + * of HMIs > > > + */ > > Ben, > > The CPU helper would be to replicate the value of the SPR_HMER in all > the threads of the core I guess ? Nope, those HMER bits are per-thread afaik, but I dislike having a device whack at CPU state... Also when we do PR KVM of powernv, we'll want to sync the SPR with KVM I suppose. Cheers, Ben.
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Tue, 2016-09-27 at 11:30 +0200, Cédric Le Goater wrote:
> On 09/27/2016 11:10 AM, Cédric Le Goater wrote:
> >
> > >
> > > >
> > > > +#include
> > > > +
> > > > +static void xscom_complete(uint64_t hmer_bits)
> > > > +{
> > > > +CPUState *cs = current_cpu;
> > >
> > > Hmm.. is current_cpu a safe thing to use in the case of KVM or MTTCG?
> >
> > yes, as we are running under cpu_exec when doing this call.
>
> well, this is not true under the monitor.
>
> So we will have to come up with something to handle xscom read/writes
> from the monitor. Could we use first_cpu in that case ?
Well, we'll need to find the chip etc... I think the XSCOM bridge (or bus)
should have special methods for the monitor that don't update HMER but
print out the status instead.
Cheers,
Ben.
> Thanks,
>
> C.
>
> >
> > >
> > > >
> > > > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > > +CPUPPCState *env = &cpu->env;
> > > > +
> > > > +cpu_synchronize_state(cs);
> > > > +env->spr[SPR_HMER] |= hmer_bits;
> > > > +
> > > > +/* XXX Need a CPU helper to set HMER, also handle gneeration
> > > > + * of HMIs
> > > > + */
> >
> > Ben,
> >
> > The CPU helper would be to replicate the value of the SPR_HMER in all
> > the threads of the core I guess ?
> >
> > Thanks,
> >
> > C.
> >
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On 09/27/2016 11:10 AM, Cédric Le Goater wrote:
>>> +#include
>>> +
>>> +static void xscom_complete(uint64_t hmer_bits)
>>> +{
>>> +CPUState *cs = current_cpu;
>>
>> Hmm.. is current_cpu a safe thing to use in the case of KVM or MTTCG?
>
> yes, as we are running under cpu_exec when doing this call.
well, this is not true under the monitor.
So we will have to come up with something to handle xscom read/writes
from the monitor. Could we use first_cpu in that case ?
Thanks,
C.
>>> +PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +CPUPPCState *env = &cpu->env;
>>> +
>>> +cpu_synchronize_state(cs);
>>> +env->spr[SPR_HMER] |= hmer_bits;
>>> +
>>> +/* XXX Need a CPU helper to set HMER, also handle gneeration
>>> + * of HMIs
>>> + */
>
> Ben,
>
> The CPU helper would be to replicate the value of the SPR_HMER in all
> the threads of the core I guess ?
>
> Thanks,
>
> C.
>
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
>> +#include
>> +
>> +static void xscom_complete(uint64_t hmer_bits)
>> +{
>> +CPUState *cs = current_cpu;
>
> Hmm.. is current_cpu a safe thing to use in the case of KVM or MTTCG?
yes, as we are running under cpu_exec when doing this call.
>> +PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +CPUPPCState *env = &cpu->env;
>> +
>> +cpu_synchronize_state(cs);
>> +env->spr[SPR_HMER] |= hmer_bits;
>> +
>> +/* XXX Need a CPU helper to set HMER, also handle gneeration
>> + * of HMIs
>> + */
Ben,
The CPU helper would be to replicate the value of the SPR_HMER in all
the threads of the core I guess ?
Thanks,
C.
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On 09/27/2016 08:10 AM, Benjamin Herrenschmidt wrote: > On Tue, 2016-09-27 at 07:54 +0200, Cédric Le Goater wrote: >> >>> but I guess if you have the decoding of those "core" registers >>> here as well, then that doesn't make so much sense. > > Those core registers may well change with P9, we havne't looked closely > yet... Neither have I ... qemu/pnv reaches the kernel but this is really a "Frankenstein P9". The interrupt controller is missing. C. >> yes and there is also the handling of the XSCOM failures. >> >> 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,
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Tue, 2016-09-27 at 07:54 +0200, Cédric Le Goater wrote: > > > but I guess if you have the decoding of those "core" registers > > here as well, then that doesn't make so much sense. Those core registers may well change with P9, we havne't looked closely yet... > yes and there is also the handling of the XSCOM failures. > > 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,
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
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 = 0x100d10498000ull; /* 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. 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.
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..
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
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 = 0x100d10498000ull; /* 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, but I guess if
you have the decoding of those "core" registers here as well, then
that doesn't make so much sense.
> 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;
> +uint3
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
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 = 0x100d10498000ull; /* 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;
+
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
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 = 0x100d10498000ull; /* 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.
>
>
> >> dc->desc = "PowerNV Chip POWER9";
> >> }
> >>
> >> @@ -527,6 +561,16 @@ static void pnv_chip_core_sanitize(PnvChip *chip)
> >> chip->cores_mask &= pcc->cores_mask;
> >> }
> >>
> >> +static void pnv_chip_init(Object *obj)
> >> +{
> >> +PnvChip *chip = PNV_CHIP(obj);
> >> +
> >> +object_initialize(&chip->xscom, sizeof(chip->xscom), TYPE_PNV_XSCOM);
> >> +object_property_add_child(obj, "xscom", OBJECT(&chip->xscom), NULL);
> >> +object_property_add_const_link(OBJECT(&chip->xscom), "chip",
> >> + OBJECT(chip), &error_abort);
> >> +}
> >> +
> >> static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >> {
> >> PnvChip *chip = PNV_CHIP(dev);
> >> @@ -540,6 +584,12 @@ static void pnv_chip_realize(DeviceState *dev, Error
> >> **errp)
> >> return;
> >> }
> >>
> >> +/* XSCOM bridge */
> >> +object_property_set_bool(OBJECT(&chip->xscom), true, "realized",
> >> + &error_fatal);
> >> +sysbus_mmio_map(SYS_BUS_DEVICE(&chip->xscom), 0,
> >> +PNV_XSCOM_BASE(chip->chip_id));
> >> +
> >> /* Early checks on the core settings */
> >> pnv_chip_core_sanitize(chip);
> >>
> >> @@ -597,6 +647,7 @@ static const TypeInfo pnv_chip_info = {
> >> .name = TYPE_PNV_CHIP,
> >> .parent= TYPE_SYS_BUS_DEVICE,
> >> .class_init= pnv_chip_class_init,
> >> +.instance_init = pnv_chip_init,
> >> .class_size= sizeof(PnvChipClass),
> >> .abstract = true,
> >> };
> >> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> >> new file mode 100644
> >> index ..019cd85428de
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_xscom.c
> >> @@ -0,0 +1,308 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV XSCOM bus
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation.
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see
> >>
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
>> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectClass
>> *klass, void *data)
>> k->chip_cfam_id = 0x100d10498000ull; /* 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.
>> dc->desc = "PowerNV Chip POWER9";
>> }
>>
>> @@ -527,6 +561,16 @@ static void pnv_chip_core_sanitize(PnvChip *chip)
>> chip->cores_mask &= pcc->cores_mask;
>> }
>>
>> +static void pnv_chip_init(Object *obj)
>> +{
>> +PnvChip *chip = PNV_CHIP(obj);
>> +
>> +object_initialize(&chip->xscom, sizeof(chip->xscom), TYPE_PNV_XSCOM);
>> +object_property_add_child(obj, "xscom", OBJECT(&chip->xscom), NULL);
>> +object_property_add_const_link(OBJECT(&chip->xscom), "chip",
>> + OBJECT(chip), &error_abort);
>> +}
>> +
>> static void pnv_chip_realize(DeviceState *dev, Error **errp)
>> {
>> PnvChip *chip = PNV_CHIP(dev);
>> @@ -540,6 +584,12 @@ static void pnv_chip_realize(DeviceState *dev, Error
>> **errp)
>> return;
>> }
>>
>> +/* XSCOM bridge */
>> +object_property_set_bool(OBJECT(&chip->xscom), true, "realized",
>> + &error_fatal);
>> +sysbus_mmio_map(SYS_BUS_DEVICE(&chip->xscom), 0,
>> +PNV_XSCOM_BASE(chip->chip_id));
>> +
>> /* Early checks on the core settings */
>> pnv_chip_core_sanitize(chip);
>>
>> @@ -597,6 +647,7 @@ static const TypeInfo pnv_chip_info = {
>> .name = TYPE_PNV_CHIP,
>> .parent= TYPE_SYS_BUS_DEVICE,
>> .class_init= pnv_chip_class_init,
>> +.instance_init = pnv_chip_init,
>> .class_size= sizeof(PnvChipClass),
>> .abstract = true,
>> };
>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>> new file mode 100644
>> index ..019cd85428de
>> --- /dev/null
>> +++ b/hw/ppc/pnv_xscom.c
>> @@ -0,0 +1,308 @@
>> +/*
>> + * QEMU PowerPC PowerNV XSCOM bus
>> + *
>> + * Copyright (c) 2016, IBM Corporation.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see
>>
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Wed, 2016-09-21 at 15:56 +1000, David Gibson wrote: > > Yes, I think that's the way to go. > > That also means on P9 you can potentially just map the scom address > space directly into address_space_memory, instead of requiring a > redispatcher to do the address mangling. No. You still need a redispatcher to do the HMER business Cheers, Ben.
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Thu, Sep 15, 2016 at 02:45:57PM +0200, Cédric Le Goater wrote: > On a real POWER8 system, the Pervasive Interconnect Bus (PIB) serves > as a backbone to connect different units of the system. The host > firmware connects to the PIB through a bridge unit, the > Alter-Display-Unit (ADU), which gives him access to all the chiplets > on the PCB network (Pervasive Connect Bus), the PIB acting as the root > of this network. > > XSCOM (serial communication) is the interface to the sideband bus > provided by the POWER8 pervasive unit to read and write to chiplets > resources. This is needed by the host firmware, OPAL and to a lesser > extent, Linux. This is among others how the PCI Host bridges get > configured at boot or how the LPC bus is accessed. > > The PnvXScom object represents the ADU of a real system, it dispatches > XSCOM accesses to the targeted chiplets using a specific XSCOM address > space introduced for this purpose. A set of handlers to translate of > an XSCOM address into a PCB register address is added to the PnvChip > class to handle the differences between P9 and P8. > > Unfortunately, due to the nature of the translation, we can not > translate before the dispatch in the address space. So an address > relative to the XSCOM mapping in main memory is used and the > translation is done in the XSCOM memory subregions with helpers. > > To customize the device tree, a QOM InterfaceClass, PnvXScomInterface, > is provided with a devnode() handler. The device tree is populated by > simply looping on the PnvXScom children. Therefore, each model needing > custom nodes should declare itself as a child of the PnvXScom object > at instantiation time. > > The PnvXScomInterface is also used to hold the address translation > handlers required by models having XSCOM memory subregions. > > *Caveats* > > - I kept a standalone for PnvXScom object in this model to isolate >the feature but it should be merged with the PnvChip and remove >quite a few lines at the same time. This is minor. Ok. > - The PCB translation is too much of a constraint for a specific >XSCOM address space, unless someone can explain me how to address 8 >bytes at 0xb0021 and another 8 different bytes at 0xb0022. I don't >think the address space and the memory regions were designed with >this in mind. Please advise ! > >So we should problably just kill the idea of a specific address >space and use helpers in the XSCOM memory subregions to translate >XSCOM addresses to PCB registers. > >We can improve the calls to initialize and to map the XSCOM subregions, >memory_region_add_subregion() and memory_region_init_io(), with >wrappers and make the translation a bit less painful. > > - The way the translation helpers of the PnvXScomInterface are >initialized is a bit of a hack. Check : > > uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr) > uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba) > >Ideas welcomed ! > > Based on previous work done by : > Benjamin Herrenschmidt > > Signed-off-by: Cédric Le Goater > --- > > Changes since v2: > > - QOMified the model. > - all mappings in main memory space are now gathered in >pnv_chip_realize() as done on other architectures. > - removed XScomBus. The parenthood is established through the QOM >model > - replaced the XScomDevice with an InterfaceClass : PnvXScomInterface. > - introduced an XSCOM address space to dispatch accesses to the >chiplets > > hw/ppc/Makefile.objs | 2 +- > hw/ppc/pnv.c | 51 > hw/ppc/pnv_xscom.c | 308 > + > include/hw/ppc/pnv.h | 4 + > include/hw/ppc/pnv_xscom.h | 74 +++ > 5 files changed, 438 insertions(+), 1 deletion(-) > create mode 100644 hw/ppc/pnv_xscom.c > create mode 100644 include/hw/ppc/pnv_xscom.h > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs > index f8c7d1db9ade..08c213c40684 100644 > --- a/hw/ppc/Makefile.objs > +++ b/hw/ppc/Makefile.objs > @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o > spapr_rtas.o > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o > # IBM PowerNV > -obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o > +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o > ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) > obj-y += spapr_pci_vfio.o > endif > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 7bc98f15f14b..7dcdf18a9e6b 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -32,6 +32,8 @@ > #include "exec/address-spaces.h" > #include "qemu/cutils.h" > > +#include "hw/ppc/pnv_xscom.h" > + > #include > > #define FDT_MAX_SIZE0x0010 > @@ -218,6 +220,8 @@ static void powernv_populate_chip(PnvChip *chip, void > *fdt) > size_t typesize = object_type_get_instance_size(typ
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Fri, Sep 16, 2016 at 08:11:45AM +1000, Benjamin Herrenschmidt wrote: > On Thu, 2016-09-15 at 14:45 +0200, Cédric Le Goater wrote: > > - The PCB translation is too much of a constraint for a specific > > XSCOM address space, unless someone can explain me how to address 8 > > bytes at 0xb0021 and another 8 different bytes at 0xb0022. I don't > > think the address space and the memory regions were designed with > > this in mind. Please advise ! > > I'd say just dispatch pcb_addr << 3 to the memory regions (which is > also the P9 translation iirc). Yes, I think that's the way to go. That also means on P9 you can potentially just map the scom address space directly into address_space_memory, instead of requiring a redispatcher to do the address mangling. > Just add a quirk to the ADU/XSCOM dispatch object to do the additional > unmangling needed on P7/P8 > -- 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure
On Thu, 2016-09-15 at 14:45 +0200, Cédric Le Goater wrote: > - The PCB translation is too much of a constraint for a specific > XSCOM address space, unless someone can explain me how to address 8 > bytes at 0xb0021 and another 8 different bytes at 0xb0022. I don't > think the address space and the memory regions were designed with > this in mind. Please advise ! I'd say just dispatch pcb_addr << 3 to the memory regions (which is also the P9 translation iirc). Just add a quirk to the ADU/XSCOM dispatch object to do the additional unmangling needed on P7/P8 Cheers, Ben.
