On 03/10/2019 04:08, David Gibson wrote:
> On Wed, Sep 18, 2019 at 06:06:31PM +0200, Cédric Le Goater wrote:
>> This also removes the need of the get_tctx() XiveRouter handler in the
>> core XIVE framework.
>
> In general these commit messages could really do with more context.
> What exactly is the "controller model"? Where were the TIMA
> operations before now. Why is having them in the controller model
> better?
>
> I could probably answer those with some investigation, but the point
> is that the commit message is supposed to make it easy to find that
> information.
>
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> include/hw/ppc/xive.h | 1 -
>> hw/intc/pnv_xive.c | 35 ++++++++++++++++++++++++++++++++++-
>> hw/intc/spapr_xive.c | 33 +++++++++++++++++++++++++++++++--
>> hw/intc/xive.c | 29 -----------------------------
>> 4 files changed, 65 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 536deea8c622..9d9cd88dd17e 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -462,7 +462,6 @@ typedef struct XiveENDSource {
>> #define XIVE_TM_OS_PAGE 0x2
>> #define XIVE_TM_USER_PAGE 0x3
>>
>> -extern const MemoryRegionOps xive_tm_ops;
>> void xive_tctx_tm_write(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
>> uint64_t value, unsigned size);
>> uint64_t xive_tctx_tm_read(XivePresenter *xptr, XiveTCTX *tctx, hwaddr
>> offset,
>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>> index 3d6fcf9ac139..40e18fb44811 100644
>> --- a/hw/intc/pnv_xive.c
>> +++ b/hw/intc/pnv_xive.c
>> @@ -1475,6 +1475,39 @@ static const MemoryRegionOps xive_tm_indirect_ops = {
>> },
>> };
>>
>> +static void pnv_xive_tm_write(void *opaque, hwaddr offset,
>> + uint64_t value, unsigned size)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> + PnvXive *xive = pnv_xive_tm_get_xive(cpu);
>> + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>> +
>> + xive_tctx_tm_write(XIVE_PRESENTER(xive), tctx, offset, value, size);
>> +}
>> +
>> +static uint64_t pnv_xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> + PnvXive *xive = pnv_xive_tm_get_xive(cpu);
>> + XiveTCTX *tctx = XIVE_TCTX(pnv_cpu_state(cpu)->intc);
>> +
>> + return xive_tctx_tm_read(XIVE_PRESENTER(xive), tctx, offset, size);
>
> You're not using the opaque pointer in either of these cases. So
> couldn't you make it point to the presenter for pnv as well, then...
On PowerNV, it's the cpu doing the TIMA load/store which determines
the chip and the XiveTCTX is deduced from the pnv_cpu_state().
The TIMA is only mapped on chip0 in our model. See CQ_TM1_BAR reg.
>
>> +}
>> +
>> +const MemoryRegionOps pnv_xive_tm_ops = {
>> + .read = pnv_xive_tm_read,
>> + .write = pnv_xive_tm_write,
>> + .endianness = DEVICE_BIG_ENDIAN,
>> + .valid = {
>> + .min_access_size = 1,
>> + .max_access_size = 8,
>> + },
>> + .impl = {
>> + .min_access_size = 1,
>> + .max_access_size = 8,
>> + },
>> +};
>> +
>> /*
>> * Interrupt controller XSCOM region.
>> */
>> @@ -1832,7 +1865,7 @@ static void pnv_xive_realize(DeviceState *dev, Error
>> **errp)
>> "xive-pc", PNV9_XIVE_PC_SIZE);
>>
>> /* Thread Interrupt Management Area (Direct) */
>> - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops,
>> + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops,
>> xive, "xive-tima", PNV9_XIVE_TM_SIZE);
>>
>> qemu_register_reset(pnv_xive_reset, dev);
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index eefc0d4c36b9..e00a9bdd901b 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -222,6 +222,35 @@ void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
>> memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
>> }
>>
>> +static void spapr_xive_tm_write(void *opaque, hwaddr offset,
>> + uint64_t value, unsigned size)
>> +{
>> + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx;
>> +
>> + xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value,
>> size);
>
> ... there would be no difference from the spapr versions, AFAICT.
the XiveTCTX is deduced from the spapr_cpu_state().
C.
>
>> +}
>> +
>> +static uint64_t spapr_xive_tm_read(void *opaque, hwaddr offset, unsigned
>> size)
>> +{
>> + XiveTCTX *tctx = spapr_cpu_state(POWERPC_CPU(current_cpu))->tctx;
>> +
>> + return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size);
>> +}
>> +
>> +const MemoryRegionOps spapr_xive_tm_ops = {
>> + .read = spapr_xive_tm_read,
>> + .write = spapr_xive_tm_write,
>> + .endianness = DEVICE_BIG_ENDIAN,
>> + .valid = {
>> + .min_access_size = 1,
>> + .max_access_size = 8,
>> + },
>> + .impl = {
>> + .min_access_size = 1,
>> + .max_access_size = 8,
>> + },
>> +};
>> +
>> static void spapr_xive_end_reset(XiveEND *end)
>> {
>> memset(end, 0, sizeof(*end));
>> @@ -331,8 +360,8 @@ static void spapr_xive_realize(DeviceState *dev, Error
>> **errp)
>> qemu_register_reset(spapr_xive_reset, dev);
>>
>> /* TIMA initialization */
>> - memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
>> - "xive.tima", 4ull << TM_SHIFT);
>> + memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &spapr_xive_tm_ops,
>> + xive, "xive.tima", 4ull << TM_SHIFT);
>> sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>>
>> /*
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 9bb09ed6ee7b..11432f04f5c3 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -483,35 +483,6 @@ uint64_t xive_tctx_tm_read(XivePresenter *xptr,
>> XiveTCTX *tctx, hwaddr offset,
>> return xive_tm_raw_read(tctx, offset, size);
>> }
>>
>> -static void xive_tm_write(void *opaque, hwaddr offset,
>> - uint64_t value, unsigned size)
>> -{
>> - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu);
>> -
>> - xive_tctx_tm_write(XIVE_PRESENTER(opaque), tctx, offset, value, size);
>> -}
>> -
>> -static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>> -{
>> - XiveTCTX *tctx = xive_router_get_tctx(XIVE_ROUTER(opaque), current_cpu);
>> -
>> - return xive_tctx_tm_read(XIVE_PRESENTER(opaque), tctx, offset, size);
>> -}
>> -
>> -const MemoryRegionOps xive_tm_ops = {
>> - .read = xive_tm_read,
>> - .write = xive_tm_write,
>> - .endianness = DEVICE_BIG_ENDIAN,
>> - .valid = {
>> - .min_access_size = 1,
>> - .max_access_size = 8,
>> - },
>> - .impl = {
>> - .min_access_size = 1,
>> - .max_access_size = 8,
>> - },
>> -};
>> -
>> static char *xive_tctx_ring_print(uint8_t *ring)
>> {
>> uint32_t w2 = xive_tctx_word2(ring);
>
