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

[Cédric Le Goater]

On 4/2/20 12:51 PM, Greg Kurz wrote:
> On Wed,  1 Apr 2020 18:46:53 +0200
> Cédric Le Goater <address@hidden> 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 <address@hidden>
>> ---
>>  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.

I will simply implement the hooks for pseries. No big deal.

> 
>>  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 


It should never happen because the call above to xive_router_get_eas() would
have errored.

C.


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