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

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

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

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 <clg@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...

> +}
> +
> +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.

> +}
> +
> +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);

-- 
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: 833 bytes --]