Re: [PATCH v4 11/25] ppc/xive: Move the TIMA operations to the controller model

[Cédric Le Goater]

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 <address@hidden>
>> ---
>>  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);
>