Re: [PATCH v2 2/2] ppc/xive: Add support for PQ state bits offload

[Greg Kurz <groug@kaod.org>]

On Wed,  1 Apr 2020 18:46:53 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> The trigger message coming from a HW source contains a special bit
> informing the XIVE interrupt controller that the PQ bits have been
> checked at the source or not. Depending on the value, the IC can
> perform the check and the state transition locally using its own PQ
> state bits.
> 
> The following changes add new accessors to the XiveRouter required to
> query and update the PQ state bits. This is only applies to the
> PowerNV machine, sPAPR is not concerned by such complex configuration.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/xive.h  |  8 +++++--
>  hw/intc/pnv_xive.c     | 37 +++++++++++++++++++++++++++++---
>  hw/intc/xive.c         | 48 ++++++++++++++++++++++++++++++++++++------
>  hw/pci-host/pnv_phb4.c |  9 ++++++--
>  hw/ppc/pnv_psi.c       |  8 +++++--
>  5 files changed, 94 insertions(+), 16 deletions(-)
> 
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 112fb6fb6dbe..050e49c14fd9 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -160,7 +160,7 @@ typedef struct XiveNotifier XiveNotifier;
>  
>  typedef struct XiveNotifierClass {
>      InterfaceClass parent;
> -    void (*notify)(XiveNotifier *xn, uint32_t lisn);
> +    void (*notify)(XiveNotifier *xn, uint32_t lisn, bool pq_checked);
>  } XiveNotifierClass;
>  
>  /*
> @@ -354,6 +354,10 @@ typedef struct XiveRouterClass {
>      /* XIVE table accessors */
>      int (*get_eas)(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
>                     XiveEAS *eas);
> +    int (*get_pq)(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
> +                  uint8_t *pq);
> +    int (*set_pq)(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
> +                  uint8_t *pq);
>      int (*get_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>                     XiveEND *end);
>      int (*write_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> @@ -375,7 +379,7 @@ int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>                          XiveNVT *nvt);
>  int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>                            XiveNVT *nvt, uint8_t word_number);
> -void xive_router_notify(XiveNotifier *xn, uint32_t lisn);
> +void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked);
>  
>  /*
>   * XIVE Presenter
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 77cacdd6c623..bcfe9dc54e3b 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -373,6 +373,34 @@ static int pnv_xive_get_eas(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
>      return pnv_xive_vst_read(xive, VST_TSEL_IVT, blk, idx, eas);
>  }
>  
> +static int pnv_xive_get_pq(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
> +                           uint8_t *pq)
> +{
> +    PnvXive *xive = PNV_XIVE(xrtr);
> +
> +    if (pnv_xive_block_id(xive) != blk) {
> +        xive_error(xive, "VST: EAS %x is remote !?", XIVE_EAS(blk, idx));
> +        return -1;
> +    }
> +
> +    *pq = xive_source_esb_get(&xive->ipi_source, idx);
> +    return 0;
> +}
> +
> +static int pnv_xive_set_pq(XiveRouter *xrtr, uint8_t blk, uint32_t idx,
> +                           uint8_t *pq)
> +{
> +    PnvXive *xive = PNV_XIVE(xrtr);
> +
> +    if (pnv_xive_block_id(xive) != blk) {
> +        xive_error(xive, "VST: EAS %x is remote !?", XIVE_EAS(blk, idx));
> +        return -1;
> +    }
> +
> +    *pq = xive_source_esb_set(&xive->ipi_source, idx, *pq);
> +    return 0;
> +}
> +
>  /*
>   * One bit per thread id. The first register PC_THREAD_EN_REG0 covers
>   * the first cores 0-15 (normal) of the chip or 0-7 (fused). The
> @@ -469,12 +497,12 @@ static PnvXive *pnv_xive_tm_get_xive(PowerPCCPU *cpu)
>   * event notification to the Router. This is required on a multichip
>   * system.
>   */
> -static void pnv_xive_notify(XiveNotifier *xn, uint32_t srcno)
> +static void pnv_xive_notify(XiveNotifier *xn, uint32_t srcno, bool pq_checked)
>  {
>      PnvXive *xive = PNV_XIVE(xn);
>      uint8_t blk = pnv_xive_block_id(xive);
>  
> -    xive_router_notify(xn, XIVE_EAS(blk, srcno));
> +    xive_router_notify(xn, XIVE_EAS(blk, srcno), pq_checked);
>  }
>  
>  /*
> @@ -1316,7 +1344,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive, hwaddr addr, uint64_t val)
>      blk = XIVE_EAS_BLOCK(val);
>      idx = XIVE_EAS_INDEX(val);
>  
> -    xive_router_notify(XIVE_NOTIFIER(xive), XIVE_EAS(blk, idx));
> +    xive_router_notify(XIVE_NOTIFIER(xive), XIVE_EAS(blk, idx),
> +                       !!(val & XIVE_TRIGGER_PQ));
>  }
>  
>  static void pnv_xive_ic_notify_write(void *opaque, hwaddr addr, uint64_t val,
> @@ -1944,6 +1973,8 @@ static void pnv_xive_class_init(ObjectClass *klass, void *data)
>      device_class_set_props(dc, pnv_xive_properties);
>  
>      xrc->get_eas = pnv_xive_get_eas;
> +    xrc->get_pq = pnv_xive_get_pq;
> +    xrc->set_pq = pnv_xive_set_pq;
>      xrc->get_end = pnv_xive_get_end;
>      xrc->write_end = pnv_xive_write_end;
>      xrc->get_nvt = pnv_xive_get_nvt;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b8825577f719..894be4b49ba4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -927,7 +927,7 @@ static void xive_source_notify(XiveSource *xsrc, int srcno)
>      XiveNotifierClass *xnc = XIVE_NOTIFIER_GET_CLASS(xsrc->xive);
>  
>      if (xnc->notify) {
> -        xnc->notify(xsrc->xive, srcno);
> +        xnc->notify(xsrc->xive, srcno, true);
>      }
>  }
>  
> @@ -1343,6 +1343,24 @@ int xive_router_get_eas(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
>      return xrc->get_eas(xrtr, eas_blk, eas_idx, eas);
>  }
>  
> +static
> +int xive_router_get_pq(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
> +                       uint8_t *pq)
> +{
> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
> +
> +    return xrc->get_pq(xrtr, eas_blk, eas_idx, pq);
> +}
> +
> +static
> +int xive_router_set_pq(XiveRouter *xrtr, uint8_t eas_blk, uint32_t eas_idx,
> +                       uint8_t *pq)
> +{
> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
> +
> +    return xrc->set_pq(xrtr, eas_blk, eas_idx, pq);
> +}
> +

Only the powernv machine implements the PQ bits hooks. I understand
that pseries doesn't need them because it cannot execute the helpers
above in practice, since xive_router_notify() is always called with
pq_checked == true :

spapr_xive_set_irq()
 xive_source_set_irq()
  xive_source_notify()
  {
     xnc->notify(xsrc->xive, srcno, true);
  }

IMHO the XIVE code is complex enough that we should maybe make
this really explicit with a comment and assert(xrc->[sg]et_pq)
in the PQ bits helpers.

>  int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>                          XiveEND *end)
>  {
> @@ -1686,7 +1704,7 @@ do_escalation:
>                             xive_get_field32(END_W5_ESC_END_DATA,  end.w5));
>  }
>  
> -void xive_router_notify(XiveNotifier *xn, uint32_t lisn)
> +void xive_router_notify(XiveNotifier *xn, uint32_t lisn, bool pq_checked)
>  {
>      XiveRouter *xrtr = XIVE_ROUTER(xn);
>      uint8_t eas_blk = XIVE_EAS_BLOCK(lisn);
> @@ -1699,11 +1717,27 @@ void xive_router_notify(XiveNotifier *xn, uint32_t lisn)
>          return;
>      }
>  
> -    /*
> -     * The IVRE checks the State Bit Cache at this point. We skip the
> -     * SBC lookup because the state bits of the sources are modeled
> -     * internally in QEMU.
> -     */
> +    if (!pq_checked) {
> +        bool notify;
> +        uint8_t pq;
> +
> +        /* PQ cache lookup */
> +        if (xive_router_get_pq(xrtr, eas_blk, eas_idx, &pq)) {
> +            /* Set FIR */
> +            g_assert_not_reached();

I'm not sure about the intent of this assert... Actual QEMU
bug that should never happen ever or guest misuse of XIVE
that we might try to handle one day ?

In the former case, I'd rather assert() in the PQ bits
machine hooks when the error condition is first detected.

Apart from that, rest looks good.

> +        }
> +
> +        notify = xive_esb_trigger(&pq);
> +
> +        if (xive_router_set_pq(xrtr, eas_blk, eas_idx, &pq)) {
> +            /* Set FIR */
> +            g_assert_not_reached();
> +        }
> +
> +        if (!notify) {
> +            return;
> +        }
> +    }
>  
>      if (!xive_eas_is_valid(&eas)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %x\n", lisn);
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ac824f877cf1..c451819bfd52 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1241,14 +1241,19 @@ static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
>      return phb->bus_path;
>  }
>  
> -static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno)
> +static void pnv_phb4_xive_notify(XiveNotifier *xf, uint32_t srcno,
> +                                 bool pq_checked)
>  {
>      PnvPHB4 *phb = PNV_PHB4(xf);
>      uint64_t notif_port = phb->regs[PHB_INT_NOTIFY_ADDR >> 3];
>      uint32_t offset = phb->regs[PHB_INT_NOTIFY_INDEX >> 3];
> -    uint64_t data = XIVE_TRIGGER_PQ | offset | srcno;
> +    uint64_t data = offset | srcno;
>      MemTxResult result;
>  
> +    if (pq_checked) {
> +        data |= XIVE_TRIGGER_PQ;
> +    }
> +
>      address_space_stq_be(&address_space_memory, notif_port, data,
>                           MEMTXATTRS_UNSPECIFIED, &result);
>      if (result != MEMTX_OK) {
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index c34a49b000f2..63804f28f45e 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -652,7 +652,7 @@ static const TypeInfo pnv_psi_power8_info = {
>  #define   PSIHB9_IRQ_STAT_DIO           PPC_BIT(12)
>  #define   PSIHB9_IRQ_STAT_PSU           PPC_BIT(13)
>  
> -static void pnv_psi_notify(XiveNotifier *xf, uint32_t srcno)
> +static void pnv_psi_notify(XiveNotifier *xf, uint32_t srcno, bool pq_checked)
>  {
>      PnvPsi *psi = PNV_PSI(xf);
>      uint64_t notif_port = psi->regs[PSIHB_REG(PSIHB9_ESB_NOTIF_ADDR)];
> @@ -661,9 +661,13 @@ static void pnv_psi_notify(XiveNotifier *xf, uint32_t srcno)
>  
>      uint32_t offset =
>          (psi->regs[PSIHB_REG(PSIHB9_IVT_OFFSET)] >> PSIHB9_IVT_OFF_SHIFT);
> -    uint64_t data = XIVE_TRIGGER_PQ | offset | srcno;
> +    uint64_t data = offset | srcno;
>      MemTxResult result;
>  
> +    if (pq_checked) {
> +        data |= XIVE_TRIGGER_PQ;
> +    }
> +
>      if (!valid) {
>          return;
>      }