Re: [Qemu-devel] [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier

[David Gibson]

On Wed, Nov 28, 2018 at 05:48:32PM +0100, Cédric Le Goater wrote:
> On 11/28/18 3:39 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:04AM +0100, Cédric Le Goater wrote:
> >> The IVPE scans the O/S CAM line of the XIVE thread interrupt contexts
> >> to find a matching Notification Virtual Target (NVT) among the NVTs
> >> dispatched on the HW processor threads.
> >>
> >> On a real system, the thread interrupt contexts are updated by the
> >> hypervisor when a Virtual Processor is scheduled to run on a HW
> >> thread. Under QEMU, the model emulates the same behavior by hardwiring
> >> the NVT identifier in the thread context registers at reset.
> >>
> >> The NVT identifier used by the sPAPRXive model is the VCPU id. The END
> >> identifier is also derived from the VCPU id. A set of helpers doing
> >> the conversion between identifiers are provided for the hcalls
> >> configuring the sources and the ENDs.
> >>
> >> The model does not need a NVT table but The XiveRouter NVT operations
> >> are provided to perform some extra checks in the routing algorithm.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/spapr_xive.h |  17 +++++
> >>  include/hw/ppc/xive.h       |   3 +
> >>  hw/intc/spapr_xive.c        | 136 ++++++++++++++++++++++++++++++++++++
> >>  hw/intc/xive.c              |   9 +++
> >>  4 files changed, 165 insertions(+)
> >>
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 06727bd86aa9..3f65b8f485fd 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -43,4 +43,21 @@ bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t 
> >> lisn);
> >>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> >>  
> >> +/*
> >> + * sPAPR NVT and END indexing helpers
> >> + */
> >> +uint32_t spapr_xive_nvt_to_target(sPAPRXive *xive, uint8_t nvt_blk,
> >> +                                  uint32_t nvt_idx);
> >> +int spapr_xive_target_to_nvt(sPAPRXive *xive, uint32_t target,
> >> +                            uint8_t *out_nvt_blk, uint32_t *out_nvt_idx);
> >> +int spapr_xive_cpu_to_nvt(sPAPRXive *xive, PowerPCCPU *cpu,
> >> +                          uint8_t *out_nvt_blk, uint32_t *out_nvt_idx);
> >> +
> >> +int spapr_xive_end_to_target(sPAPRXive *xive, uint8_t end_blk, uint32_t 
> >> end_idx,
> >> +                             uint32_t *out_server, uint8_t *out_prio);
> >> +int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t target, uint8_t 
> >> prio,
> >> +                             uint8_t *out_end_blk, uint32_t *out_end_idx);
> >> +int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
> >> +                          uint8_t *out_end_blk, uint32_t *out_end_idx);
> >> +
> >>  #endif /* PPC_SPAPR_XIVE_H */
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index e715a6c6923d..e6931ddaa83f 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -187,6 +187,8 @@ typedef struct XiveRouter {
> >>  #define XIVE_ROUTER_GET_CLASS(obj)                              \
> >>      OBJECT_GET_CLASS(XiveRouterClass, (obj), TYPE_XIVE_ROUTER)
> >>  
> >> +typedef struct XiveTCTX XiveTCTX;
> >> +
> >>  typedef struct XiveRouterClass {
> >>      SysBusDeviceClass parent;
> >>  
> >> @@ -201,6 +203,7 @@ typedef struct XiveRouterClass {
> >>                     XiveNVT *nvt);
> >>      int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >>                     XiveNVT *nvt);
> >> +    void (*reset_tctx)(XiveRouter *xrtr, XiveTCTX *tctx);
> >>  } XiveRouterClass;
> >>  
> >>  void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 5d038146c08e..3bf77ace11a2 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -199,6 +199,139 @@ static int spapr_xive_set_end(XiveRouter *xrtr,
> >>      return 0;
> >>  }
> >>  
> >> +static int spapr_xive_get_nvt(XiveRouter *xrtr,
> >> +                              uint8_t nvt_blk, uint32_t nvt_idx, XiveNVT 
> >> *nvt)
> >> +{
> >> +    sPAPRXive *xive = SPAPR_XIVE(xrtr);
> >> +    uint32_t vcpu_id = spapr_xive_nvt_to_target(xive, nvt_blk, nvt_idx);
> >> +    PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
> >> +
> >> +    if (!cpu) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    /*
> >> +     * sPAPR does not maintain a NVT table. Return that the NVT is
> >> +     * valid if we have found a matching CPU
> >> +     */
> >> +    nvt->w0 = NVT_W0_VALID;
> >> +    return 0;
> >> +}
> >> +
> >> +static int spapr_xive_set_nvt(XiveRouter *xrtr,
> >> +                              uint8_t nvt_blk, uint32_t nvt_idx, XiveNVT 
> >> *nvt)
> >> +{
> >> +    /* no NVT table */
> >> +    return 0;
> >> +}
> >> +
> >> +/*
> >> + * When a Virtual Processor is scheduled to run on a HW thread, the
> >> + * hypervisor pushes its identifier in the OS CAM line. Under QEMU, we
> >> + * need to emulate the same behavior.
> >> + */
> >> +static void spapr_xive_reset_tctx(XiveRouter *xrtr, XiveTCTX *tctx)
> >> +{
> >> +    uint8_t  nvt_blk;
> >> +    uint32_t nvt_idx;
> >> +    uint32_t nvt_cam;
> >> +
> >> +    spapr_xive_cpu_to_nvt(SPAPR_XIVE(xrtr), POWERPC_CPU(tctx->cs),
> >> +                          &nvt_blk, &nvt_idx);
> >> +
> >> +    nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_tctx_cam_line(nvt_blk, 
> >> nvt_idx));
> >> +    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
> >> +}
> >> +
> >> +/*
> >> + * The allocation of VP blocks is a complex operation in OPAL and the
> >> + * VP identifiers have a relation with the number of HW chips, the
> >> + * size of the VP blocks, VP grouping, etc. The QEMU sPAPR XIVE
> >> + * controller model does not have the same constraints and can use a
> >> + * simple mapping scheme of the CPU vcpu_id
> >> + *
> >> + * These identifiers are never returned to the OS.
> >> + */
> >> +
> >> +#define SPAPR_XIVE_VP_BASE 0x400
> > 
> > 0x400 == 1024.  Could we ever have the possibility of needing to
> > consider both physical NVTs and PAPR NVTs at the same time?  
> 
> They would not be in the same CAM line: OS ring vs. PHYS ring. 

Hm.  They still inhabit the same NVT number space though, don't they?
I'm thinking about the END->NVT stage of the process here, rather than
the NVT->TCTX stage.

Oh, also, you're using "VP" here which IIUC == "NVT".  Can we
standardize on one, please.

> > If so, does this base leave enough space for the physical ones?
> 
> I only used 0x400 to map the VP identifier to the ones allocated by KVM. 
> 0x0 would be fine but to exercise the model, it's better having a different 
> base. 
> 
> >> +uint32_t spapr_xive_nvt_to_target(sPAPRXive *xive, uint8_t nvt_blk,
> >> +                                  uint32_t nvt_idx)
> >> +{
> >> +    return nvt_idx - SPAPR_XIVE_VP_BASE;
> >> +}
> >> +
> >> +int spapr_xive_cpu_to_nvt(sPAPRXive *xive, PowerPCCPU *cpu,
> >> +                          uint8_t *out_nvt_blk, uint32_t *out_nvt_idx)
> > 
> > A number of these conversions will come out a bit simpler if we pass
> > the block and index around as a single word in most places.
> 
> Yes I have to check the whole patchset first. These prototype changes
> are not too difficult in terms of code complexity but they do break
> how patches apply and PowerNV is also using the idx and blk much more 
> explicitly. the block has a meaning on bare metal. So I am a bit 
> reluctant to do so. I will check.

Yeah, based on your comments here and earlier, I'm not sure that's a
good idea any more either.

> >> +{
> >> +    XiveRouter *xrtr = XIVE_ROUTER(xive);
> >> +
> >> +    if (!cpu) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (out_nvt_blk) {
> >> +        /* For testing purpose, we could use 0 for nvt_blk */
> >> +        *out_nvt_blk = xrtr->chip_id;
> > 
> > I don't see any point using the chip_id here, which is currently
> > always set to 0 for PAPR anyway.  If we just hardwire this to 0 it
> > removes the only use here of xrtr, which will allow some further
> > simplifications in the caller, I think.
> 
> You are right about the simplification. It was one way to exercise 
> the router model and remove any shortcuts in the indexing. I kept 
> it to be sure I was not tempted to invent new ones. I think we can
> remove it before merging. 
> 
> > 
> >> +    }
> >> +
> >> +    if (out_nvt_blk) {
> >> +        *out_nvt_idx = SPAPR_XIVE_VP_BASE + cpu->vcpu_id;
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +int spapr_xive_target_to_nvt(sPAPRXive *xive, uint32_t target,
> >> +                             uint8_t *out_nvt_blk, uint32_t *out_nvt_idx)
> > 
> > I suspect some, maybe most of these conversion functions could be static.
> 
> static inline ? 

It's in a .c file so you don't need the "inline" - the compiler can
work out whether it's a good idea to inline on its own.

> > 
> >> +{
> >> +    return spapr_xive_cpu_to_nvt(xive, spapr_find_cpu(target), 
> >> out_nvt_blk,
> >> +                                 out_nvt_idx);
> >> +}
> >> +
> >> +/*
> >> + * sPAPR END indexing uses a simple mapping of the CPU vcpu_id, 8
> >> + * priorities per CPU
> >> + */
> >> +int spapr_xive_end_to_target(sPAPRXive *xive, uint8_t end_blk, uint32_t 
> >> end_idx,
> >> +                             uint32_t *out_server, uint8_t *out_prio)
> >> +{
> >> +    if (out_server) {
> >> +        *out_server = end_idx >> 3;
> >> +    }
> >> +
> >> +    if (out_prio) {
> >> +        *out_prio = end_idx & 0x7;
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
> >> +                          uint8_t *out_end_blk, uint32_t *out_end_idx)
> >> +{
> >> +    XiveRouter *xrtr = XIVE_ROUTER(xive);
> >> +
> >> +    if (!cpu) {
> >> +        return -1;
> > 
> > Is there ever a reason this would be called with cpu == NULL?  If not
> > might as well just assert() here rather than pushing the error
> > handling back to the caller.
> 
> ok. yes.
> 
> > 
> >> +    }
> >> +
> >> +    if (out_end_blk) {
> >> +        /* For testing purpose, we could use 0 for nvt_blk */
> >> +        *out_end_blk = xrtr->chip_id;
> > 
> > Again, I don't see any point to using the chip_id, which is pretty
> > meaningless for PAPR.
> > 
> >> +    }
> >> +
> >> +    if (out_end_idx) {
> >> +        *out_end_idx = (cpu->vcpu_id << 3) + prio;
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t target, uint8_t 
> >> prio,
> >> +                             uint8_t *out_end_blk, uint32_t *out_end_idx)
> >> +{
> >> +    return spapr_xive_cpu_to_end(xive, spapr_find_cpu(target), prio,
> >> +                                 out_end_blk, out_end_idx);
> >> +}
> >> +
> >>  static const VMStateDescription vmstate_spapr_xive_end = {
> >>      .name = TYPE_SPAPR_XIVE "/end",
> >>      .version_id = 1,
> >> @@ -263,6 +396,9 @@ static void spapr_xive_class_init(ObjectClass *klass, 
> >> void *data)
> >>      xrc->set_eas = spapr_xive_set_eas;
> >>      xrc->get_end = spapr_xive_get_end;
> >>      xrc->set_end = spapr_xive_set_end;
> >> +    xrc->get_nvt = spapr_xive_get_nvt;
> >> +    xrc->set_nvt = spapr_xive_set_nvt;
> >> +    xrc->reset_tctx = spapr_xive_reset_tctx;
> >>  }
> >>  
> >>  static const TypeInfo spapr_xive_info = {
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index c49932d2b799..fc6ef5895e6d 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -481,6 +481,7 @@ static uint32_t xive_tctx_hw_cam_line(XiveTCTX *tctx, 
> >> bool block_group)
> >>  static void xive_tctx_reset(void *dev)
> >>  {
> >>      XiveTCTX *tctx = XIVE_TCTX(dev);
> >> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(tctx->xrtr);
> >>  
> >>      memset(tctx->regs, 0, sizeof(tctx->regs));
> >>  
> >> @@ -495,6 +496,14 @@ static void xive_tctx_reset(void *dev)
> >>       */
> >>      tctx->regs[TM_QW1_OS + TM_PIPR] =
> >>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
> >> +
> >> +    /*
> >> +     * QEMU sPAPR XIVE only. To let the controller model reset the OS
> >> +     * CAM line with the VP identifier.
> >> +     */
> >> +    if (xrc->reset_tctx) {
> >> +        xrc->reset_tctx(tctx->xrtr, tctx);
> >> +    }
> > 
> > AFAICT this whole function is only used from PAPR, so you can just
> > move the whole thing to the papr code and avoid the hook function.
> 
> Yes we could add a loop on all CPUs and reset all the XiveTCTX from
> the machine or a spapr_irq->reset handler. We will need at some time
> anyhow.
> 
> Thanks,
> 
> C.
> 
> 
> > 
> >>  }
> >>  
> >>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
> > 
> 

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

signature.asc
Description: PGP signature